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

Benchmark and speed processing of polyglot java imports up #10899

Merged
merged 12 commits into from
Aug 29, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Aug 27, 2024

Pull Request Description

Introduces new startup "Hello World" benchmark with many imports and a performance improvement by delaying loading classes in registerPolyglotSymbol.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

Original motivation for this PR is @hubertp profilling.npss that spends 700ms in registering polyglot symbols (which means to load all the Java classes referenced by polyglot import java statements and classloading takes time):

2s

Hubert remeasured the behavior on his slow computer and here is the result:

1s

The registerPolyglotImports method is completely gone and the whole processModule takes just 1.1s (previously 1.9s). That seems like an improvement we want to integrate.

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Aug 27, 2024
@JaroslavTulach JaroslavTulach requested a review from hubertp August 27, 2024 12:58
@JaroslavTulach JaroslavTulach self-assigned this Aug 27, 2024
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Aug 28, 2024

We have two commits right now:

now we need to restructure the PR and do new benchmark measurements because of

183ca7a should contain all the necessary fixes as well as Benchmark measuring startup with import of all libraries. Benchmark is running at https://github.com/enso-org/enso/actions/runs/10596232710

44ca279 is a follow up of 183ca7a and contains also fix for Delay loading classes in registerPolyglotSymbol by using Suppliers. Benchmark will run at https://github.com/enso-org/enso/actions/runs/10596279560

@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/StartupWithImport branch from f93ee21 to 183ca7a Compare August 28, 2024 12:00
@@ -0,0 +1,9 @@
from Standard.Base import all
Copy link
Member Author

Choose a reason for hiding this comment

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

This example was obtained by running enso --new HelloExample and then the body of main was changed.

Copy link
Member Author

@JaroslavTulach JaroslavTulach Aug 28, 2024

Choose a reason for hiding this comment

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

Startup with all imports is 22% slower than startup with simple from Standard.Base import IO:

  • this is the benchmark run
  • org_enso_benchmarks_generated_Startup_hello_world_startup is 3453ms
  • org_enso_benchmarks_generated_Startup_import_world_startup is 4217ms

E.g. a typical empty IDE project wastes 22% processing modules that aren't needed for the execution.

Copy link
Member Author

Choose a reason for hiding this comment

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

The second stdlib benchmarks run has improved all the startup benchmarks, but only minimally by 4%%, 1%% and 17%% - that's below the measurement precision we have. I was expecting bigger speedup according to the VisualVM graphs.

Expected 700ms

Copy link
Member Author

@JaroslavTulach JaroslavTulach Aug 28, 2024

Choose a reason for hiding this comment

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

The new version 0fba66b removes any trace of registerPolyglotSymbol from the (10ms interval) sampling. That's a good sign. However there still seems to be getPolyglotSymbol caught in the trace two times:

getPolyglotSymbol

Maybe we could improving the startup time by using Supplier<Object> where the polyglot symbol is requested by the IrToTruffle. One more benchmarks run scheduled with 6ee9c8f

Copy link
Member Author

@JaroslavTulach JaroslavTulach Aug 29, 2024

Choose a reason for hiding this comment

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

Previous benchmark run failed. Scheduling new one for e2c7bd3

@JaroslavTulach
Copy link
Member Author

There is an ExecCompilerTest failure caused by running with micro-distribution. Fixed by 0fba66b

@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/StartupWithImport branch from 9d1f2f9 to e2c7bd3 Compare August 29, 2024 04:57
@JaroslavTulach
Copy link
Member Author

The introduction of error message for DataflowError (done in 6c00db4) causes a WarningsTest failure caused by the fact that now there is ex.getMessage() != null for DataflowError. That's good, but as warnings cannot be attached to DataflowErrors - we need a change in the test. Done in 53eece8

@JaroslavTulach JaroslavTulach changed the title Benchmark measuring startup with import of all libraries Benchmark and speed processing of polyglot java imports up Aug 29, 2024
""");
var run = module.invokeMember("eval_expression", "run");
try {
var err = run.execute(1L);
fail("Not expecting any result: " + err);
assertTrue("Returned value represents exception: " + err, err.isException());
throw err.throwException();
Copy link
Member Author

Choose a reason for hiding this comment

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

Resolving polyglot java imports lazily prevents us to emit error during compilation. Instead we get a (dataflow) Error when we execute the run function and try to resolve Random function. Such an error is then returned as a value and needs to be throwException manually.

if (type != null && v.equals(warning1) && v.equals(warning2)) {
assertEquals(
"Cannot attach warnings to Error - check it is an error",
"Standard.Base.Error.Error",
Copy link
Member Author

Choose a reason for hiding this comment

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

@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Aug 29, 2024
if (result instanceof TruffleObject) {
return result;
}
return DataflowError.withDefaultTrace(Text.create(error), this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You only check if it is a TruffleObject not if it is null which seems to be what javadoc suggests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean the Javadoc of build method that takes Supplier<TruffleObject> argument?

If typing is on, then check for null is the same as instanceof TruffleObject. But I decided to go for the latter as someone could sneak in raw Supplier and return anything. I would need to perform some complicated checks to verify the value is proper Enso interpreter value. That can be added, but for now instanceof TruffleObject is sufficient.

@JaroslavTulach JaroslavTulach merged commit 339c275 into develop Aug 29, 2024
42 of 44 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/StartupWithImport branch August 29, 2024 15:20
@JaroslavTulach
Copy link
Member Author

At the end this PR has managed to speed the new Import_Hello benchmark by 4% from 4217ms to 4063ms:

4% in Import Hello

@enso-bot
Copy link

enso-bot bot commented Aug 30, 2024

Jaroslav Tulach reports a new STANDUP for yesterday (2024-08-29):

Progress: .

Discord
Discord is great for playing games and chilling with friends, or even building a worldwide community. Customize your own space to talk, play, and hang out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants