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

Eliminate newNakedSymbol methods #11122

Closed
wants to merge 13 commits into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jan 14, 2021

These methods produce a partially initialized symbol, which is dangerous for any code
using them. It turns out that any delays in the symbol initialization we need can be
provided by the symbol's completer.

@odersky odersky marked this pull request as draft January 15, 2021 18:46
@odersky
Copy link
Contributor Author

odersky commented Jan 15, 2021

test performance please

@dottybot
Copy link
Member

performance test scheduled: 29 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/11122/ to see the changes.

Benchmarks is based on merging with master (ba70cc0)

@odersky
Copy link
Contributor Author

odersky commented Jan 17, 2021

test performance please

@dottybot
Copy link
Member

performance test scheduled: 26 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/11122/ to see the changes.

Benchmarks is based on merging with master (65b17af)

@odersky
Copy link
Contributor Author

odersky commented Jan 17, 2021

test performance please: a3b542b, 44cb461, 552fffa, 41c4ae3, 653d8b4

@dottybot
Copy link
Member

performance test scheduled for a3b542b 44cb461 552fffa 41c4ae3 653d8b4: 25 job(s) in queue, 1 running.

@odersky
Copy link
Contributor Author

odersky commented Jan 20, 2021

@liufengyun Can it be that the performance test does not come back for three days?

@liufengyun
Copy link
Contributor

@odersky There is something abnormal, I'm looking into it.

@liufengyun
Copy link
Contributor

I restarted the job for this PR yesterday, it's running since then. However, we have a significant slowdown of the benchmarks, ~5h per PR job. It might be related to the changes in the file system or related to the jmh update in #10591.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/11122/ to see the changes.

Benchmarks is based on merging with master (26398f4)

These methods produce a partially initialized symbol, which is dangerous for any code
using them. It turns out that any delays in the symbol initialization we need can be
provided by the symbol's completer.
Last step before we can make Symbols inherit from Denotations
Use SymbolImpl instead of Symbol in all Denotation class parameters.
This is necessary to avoid cyclic references when Symbols are made
subclasses of SymDenotations.
We need to go through denot when going from a Symbol to a SymDenotation.
Hence, the fact that Symbol will extend SymDenotations is not allowed to
be known even in Symbols.
This is to avoid polymorphic dispatch on name once Symbols are subtypes
of SymDenotations
@odersky
Copy link
Contributor Author

odersky commented Jan 23, 2021

test performance please

@dottybot
Copy link
Member

performance test scheduled: 43 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/11122/ to see the changes.

Benchmarks is based on merging with master (b731573)

@anatoliykmetyuk
Copy link
Contributor

@odersky what's the status on this one? Are you planning to continue working on it or can it be closed?

@odersky
Copy link
Contributor Author

odersky commented Sep 30, 2021

The first commit is now in #13641. The other refactorings complicate things and don't seem to improve performance. So that might be a dead end, or a blocker until we have better ideas.

@odersky odersky closed this Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants