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

Experimenting with some low hanging fruit in the code that parses and imports modules for the sake of DSL loading time in VScode #1863

Merged
merged 11 commits into from
Sep 20, 2023

Conversation

jurgenvinju
Copy link
Member

This draft PR addresses the profile send by @DavyLandman on the overhead of importing modules. There are
some low hanging apples and oranges in the direction of skipping layout nodes and not allocating
intermediate tree representations the size of the entire AST.

@jurgenvinju
Copy link
Member Author

@DavyLandman this should fix some of the weird module import times we've seen. I don't know if it will come into the acceptible range of VScode extension boot time, but it's worth a try since this just makes everything faster without introducing more API.

@jurgenvinju jurgenvinju self-assigned this Sep 18, 2023
@DavyLandman
Copy link
Member

I'll try to make a benchmark ready that I can run for these changes, right now it's quite involved to get everything in the right places.

@DavyLandman
Copy link
Member

I've created a small benchmark where we only trigger a long import chain of modules that have concrete syntax in them.

rascal version fast machine slow machine
v0.33.8 5.8s 22s
this PR 3.9s 18s

so yeah, this helps.

the bench was the wall-clock-time reported for:

time java -jar <rascal-jar>.jar ModuleToImport

so it loads imports the module, and runs the main (which just prints: done loading) and then exits.

@@ -369,6 +372,21 @@ public static IList getListASTArgs(ITree tree) {
}
return writer.done();
}

public static Stream<ITree> streamListASTArgs(ITree tree) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see calls to these functions?

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #1863 (a443cca) into main (774953d) will increase coverage by 0%.
The diff coverage is 76%.

@@           Coverage Diff           @@
##              main   #1863   +/-   ##
=======================================
  Coverage       49%     49%           
- Complexity    6137    6151   +14     
=======================================
  Files          660     660           
  Lines        58634   58659   +25     
  Branches      8537    8541    +4     
=======================================
+ Hits         28874   28899   +25     
- Misses       27566   27573    +7     
+ Partials      2194    2187    -7     
Files Changed Coverage Δ
...c/org/rascalmpl/interpreter/result/JavaMethod.java 73% <ø> (ø)
src/org/rascalmpl/interpreter/utils/Modules.java 97% <ø> (-1%) ⬇️
...org/rascalmpl/values/parsetrees/SymbolFactory.java 25% <54%> (+4%) ⬆️
src/org/rascalmpl/parser/ASTBuilder.java 56% <68%> (-4%) ⬇️
...rascalmpl/values/parsetrees/ProductionAdapter.java 46% <83%> (-1%) ⬇️
src/org/rascalmpl/semantics/dynamic/Import.java 56% <100%> (+<1%) ⬆️
...g/rascalmpl/values/RascalFunctionValueFactory.java 54% <100%> (+<1%) ⬆️
...org/rascalmpl/values/parsetrees/SymbolAdapter.java 28% <100%> (+<1%) ⬆️
...c/org/rascalmpl/values/parsetrees/TreeAdapter.java 33% <100%> (+<1%) ⬆️

... and 12 files with indirect coverage changes

@jurgenvinju
Copy link
Member Author

Thanks @DavyLandman ! This is not near the effect we need so I'll stare some more

@DavyLandman
Copy link
Member

DavyLandman commented Sep 19, 2023

new benchmark:

rascal version fast machine slow machine
v0.33.8 5.8s 24s
this PR round 2 4.1s 19s

so strangly, not faster than the earlier commit. I did see it stopped loading the parser generator, so why it's not faster?

@DavyLandman
Copy link
Member

I think all these changes make sense, they made it faster, and reduced the final case that would trigger the parser generator.

@jurgenvinju jurgenvinju marked this pull request as ready for review September 20, 2023 13:30
@jurgenvinju jurgenvinju merged commit 1e3218f into main Sep 20, 2023
@jurgenvinju jurgenvinju deleted the import-optimizations branch September 20, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants