8000 Replace (Par)TrieMap with ConcurrentHashMap in IncOptimizer by gzm0 · Pull Request #4974 · scala-js/scala-js · GitHub
[go: up one dir, main page]
Skip to content

Replace (Par)TrieMap with ConcurrentHashMap in IncOptimizer #4974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Apr 6, 2024
Merged

Replace (Par)TrieMap with ConcurrentHashMap in IncOptimizer #4974

merged 2 commits into from
Apr 6, 2024

Conversation

gzm0
Copy link
Contributor
@gzm0 gzm0 commented Apr 5, 2024
  • Reduces residual retained size of the IncOptimizer on the test suite
    from 166MB to 144MB.
  • ~10% batch speedup on the optimizer.

@gzm0
Copy link
Contributor Author
gzm0 commented Apr 5, 2024

plot

logger-timings.csv

@gzm0 gzm0 requested a review from sjrd April 5, 2024 15:13
sjrd
sjrd requested changes Apr 5, 2024
View reviewed changes
Copy link
Member
@sjrd sjrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice :)

I only have one real comment about the tests.

gzm0 added 2 commits April 6, 2024 12:19
@gzm0
Implement "parallel" ConcurrentHashMap forEach methods
308236f
@gzm0
Replace (Par)TrieMap with ConcurrentHashMap in IncOptimizer
d3c42e1 
- Reduces residual retained size of the IncOptimizer on the test suite
  from 166MB to 144MB.
- ~10% batch speedup on the optimizer.
@gzm0
Copy link
Contributor Author
gzm0 commented Apr 6, 2024

I was having second thoughts about implementing the supposedly parallel forEach serially unconditionally. I have added a commit (to be squashed) which disallows values of parallelismThreshold other than Long.MaxValue.

WDYT?

@gzm0 gzm0 requested a review from sjrd April 6, 2024 11:27
@sjrd
Copy link
Member
sjrd commented Apr 6, 2024

Hum I don't think that's OK. It directly goes against our policy "if it links, it's correct". Does the API guarantee any degree of parallelism? It didn't seem like it does when I looked at it. If it doesn't, always executing serially is correct.

@gzm0
Copy link
Contributor Author
gzm0 commented Apr 6, 2024

JavaDoc says:

Using a value of 1 results in maximal parallelism by partitioning into enough subtasks to fully utilize the ForkJoinPool.commonPool() that is used for all parallel computations.

So yeah, we could argue that if we ever were to implement ForkJoinPool.commonPool, we'd implement ForkJoinPool.getCommonPoolParallelism to return 1. So that in turn would mean it's OK to allow any parallelism level.

@gzm0
Copy link
Contributor Author
gzm0 commented Apr 6, 2024

I have removed the last commit again.

@gzm0 gzm0 merged commit 95c38be into scala-js:main Apr 6, 2024
@gzm0 gzm0 deleted the chm branch April 6, 2024 15:50
@gzm0 gzm0 mentioned this pull request Apr 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjrd sjrd sjrd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gzm0 @sjrd
0