Skip to content
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.

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

Migrate WithWarnings to use EnsoHashMap to speed them up significantly #8682

Closed
JaroslavTulach opened this issue Jan 5, 2024 · 15 comments · Fixed by #10555
Closed

Migrate WithWarnings to use EnsoHashMap to speed them up significantly #8682

JaroslavTulach opened this issue Jan 5, 2024 · 15 comments · Fixed by #10555
Assignees

Comments

@JaroslavTulach
Copy link
Member

This is a long time open task that keeps popping up from time to time. For example again in

The task has been hidden as a future checkbox in #5233, but apparently it is so hidden there it deserves its own issue.

WithWarnings uses EconomicMap implementation which (despite its good name) doesn't deliver on it promise as it cannot be partially evaluated and has to be hidden behind @TruffleBoundary. That slows things down by magnitudes as Greg's benchmark also indicate.

Since

there is a better way. Replace EconomicMap with direct use of EnsoHashMap (which is carefully designed to be PE friendly) and things will get lightening fast!

@enso-bot
Copy link

enso-bot bot commented Jul 16, 2024

Pavel Marek reports a new STANDUP for today (2024-07-16):

Progress: - Collect profiling from benchmarks to see the real hotspots of WithWarnings.

@enso-bot
Copy link

enso-bot bot commented Jul 17, 2024

Pavel Marek reports a new STANDUP for today (2024-07-17):

Progress: - Migrated WithWarnings to use EnsoHashMap, the performance is much worse so far, will investigate closely tomorrow. It should be finished by 2024-07-24.

@JaroslavTulach
Copy link
Member Author

the performance is much worse so far

I cannot say why that's the case. I believe your approach is correct, Pavel.

@enso-bot
Copy link

enso-bot bot commented Jul 19, 2024

Pavel Marek reports a new STANDUP for yesterday (2024-07-18):

Progress: - Continue investigation of performance issues on WithWarnings. It should be finished by 2024-07-24.

@enso-bot
Copy link

enso-bot bot commented Jul 19, 2024

Pavel Marek reports a new STANDUP for today (2024-07-19):

Progress: - Let's not remove WarningsLibrary now, it would be too much changes at once.

  • Created dedicated AppendWarningNode that handles warning attachment functionality.
    • Is a lot of manual code rewritting, since warning handling is all over the code base.
  • The goal is to rewrite all warning handling into dedicated Truffle nodes. It should be finished by 2024-07-24.

@enso-bot
Copy link

enso-bot bot commented Jul 22, 2024

Pavel Marek reports a new STANDUP for today (2024-07-22):

Progress: - Still working on the AppendWarningNode - debugging an issue with more warnings appended than expected. It should be finished by 2024-07-24.

@enso-bot
Copy link

enso-bot bot commented Jul 23, 2024

Pavel Marek reports a new STANDUP for today (2024-07-23):

Progress: - Fixed all the tests for the newly introduced AppendWarningNode.

  • I did not test the performance yet, just fixed the tests, let's first make sure that everything works as expected. It should be finished by 2024-07-24.

@enso-bot
Copy link

enso-bot bot commented Jul 24, 2024

Pavel Marek reports a new STANDUP for today (2024-07-24):

Progress: - Starting to analyze the performance. The warnings benchmark is so far twice as slow as it used to be.

@enso-bot
Copy link

enso-bot bot commented Jul 25, 2024

Pavel Marek reports a new STANDUP for today (2024-07-25):

Progress: - reassignments of warnings are the most costly and most frequent operation and they server no purpose, let's remove them.

@enso-bot
Copy link

enso-bot bot commented Jul 26, 2024

Pavel Marek reports a new STANDUP for today (2024-07-26):

Progress: - Debugging performance - looking into IGV graphs, experimenting with profiles.

  • Managed to get the performance to a better shape, but still not better than on develop. It should be finished by 2024-07-31.

@enso-bot
Copy link

enso-bot bot commented Jul 29, 2024

Pavel Marek reports a new STANDUP for today (2024-07-29):

Progress: - Discovered std lib benchmark regression (#10702)

@enso-bot
Copy link

enso-bot bot commented Jul 30, 2024

Pavel Marek reports a new STANDUP for yesterday (2024-07-29):

Progress: - JPMS help on #10714

@enso-bot
Copy link

enso-bot bot commented Jul 31, 2024

Pavel Marek reports a new STANDUP for today (2024-07-31):

Progress: - More JPMS help

@enso-bot
Copy link

enso-bot bot commented Aug 5, 2024

Pavel Marek reports a new STANDUP for today (2024-08-05):

Progress: - Finally reveal the worst bottleneck. The benchmark results are now 10x better than on develop.

  • Integrating rest of PR reviews and test failures, try to merge ASAP. It should be finished by 2024-08-07.

@enso-bot
Copy link

enso-bot bot commented Aug 6, 2024

Pavel Marek reports a new STANDUP for today (2024-08-06):

Progress: - Tests seem OK, scheduling benchmarks on this PR and tomorrow, will compare the results. It should be finished by 2024-08-07.

@mergify mergify bot closed this as completed in #10555 Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🟢 Accepted
Development

Successfully merging a pull request may close this issue.

2 participants