Check flags that are not supposed to be mutated by completion #7427

anatoliykmetyuk · 2019-10-15T12:26:46Z

No description provided.

smarter · 2019-10-15T12:33:39Z

There's some flags for which this isn't true currently: #4221
I think the best place to have these checks is in setFlag and resetFlag, these methods should have assertions somewhat similar to https://github.com/lampepfl/dotty/blob/2e2a2df963dbb4c0fce9c162f0f579834a173ed2/compiler/src/dotty/tools/dotc/core/SymDenotations.scala#L552-L554 and https://github.com/lampepfl/dotty/blob/2e2a2df963dbb4c0fce9c162f0f579834a173ed2/compiler/src/dotty/tools/dotc/core/SymDenotations.scala#L306-L308

anatoliykmetyuk · 2019-10-21T17:07:53Z

Test performance please

anatoliykmetyuk · 2019-10-24T13:12:37Z

On top #7449.

anatoliykmetyuk · 2019-10-24T13:47:49Z

Test performance please

liufengyun · 2019-10-24T14:09:24Z

test performance please

dottybot · 2019-10-24T14:10:34Z

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

anatoliykmetyuk · 2019-10-24T14:11:57Z

Was that the title case letter or am I not in the whitelist? AFAIK I was added to the bot's whitelist at some point...

liufengyun · 2019-10-24T14:57:30Z

Yes, it seems to be case-sensitive, we should improve:

https://github.com/lampepfl/bench/blob/master/bin/process#L45

dottybot · 2019-10-24T16:06:51Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (b28ce64)

anatoliykmetyuk · 2019-10-25T13:03:00Z

test performance please

dottybot · 2019-10-25T13:04:37Z

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

dottybot · 2019-10-25T14:58:45Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (1d5e6ed)

odersky

Spending a flag on an internal check is too wasteful, I think. Also, the code used in hotspots is too expensive.

I believe this suffers from too early generalization. It seems to the problem is only with Deferred, right? What exactly is the problem? Is there a cheaper way to solve it, without creating an expensive general scaffolding?

odersky · 2019-10-26T11:46:51Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

@@ -160,20 +160,34 @@ object SymDenotations {
     */
    private[dotc] final def flagsUNSAFE: FlagSet = myFlags

+    private def setMyFlags(flags: FlagSet) =
+      lazy val immutableFlags = if myInfo.isInstanceOf[SymbolLoader] then AfterLoadFlags else FromStartFlags


Every local lazy val is a boxed object, should be avoided in performance sensitive code like this.

anatoliykmetyuk · 2019-10-26T15:27:06Z

Spending a flag on an internal check is too wasteful, I think.

True, only 2 flags left. How bad will the performance impact be if we add another Long var giving us 64 more flags? If you look at the performance test charts, the three latest data points are (in chronological order):

This PR where I used a Boolean var instead of the Loading flag.
Unrelated PR
This PR where I replaced the dedicated var with a flag.

I assumed this would help performance – however, it seems it only positively impacted the performance of dotty (source changes over time) test. Other tests are roughly at the same level for the var and the flag versions. Which may mean adding another var to the SymDenotation may not cause as much of a performance impact? Or can we just fall back to using a boolean var instead of a flag?

What is the reason for introducing a new flag

This is because we have the AfterLoadFlags which are immutable after a symbol is loaded (on load time, we can modify whatever flags we want). However, loading happens on completion of symbols, and on completion we are not allowed to modify certain flags. So the rules of flag mutability are as follows:

If we are on completion, do not modify FromStartFlags
If we are on completion but loading the symbol, allow modification of all the flags
If we are on completion after loading, do not modify AfterLoadFlags

So there are three different states to track here which requires two bits. The following chunks of code are considered as symbol loading (between startedLoading() and finishedLoading() method calls).

Loading of symbols from classfiles: https://github.com/lampepfl/dotty/pull/7427/files#diff-ef84bd1ce9ffda978ae9ae3426c1cf1dR396-R409
The late compilation, as far as I understand this is for loading symbols from the Java sources: https://github.com/lampepfl/dotty/pull/7427/files#diff-ef84bd1ce9ffda978ae9ae3426c1cf1dR419-R421
When we replace an existing symbol from the tree unpickler: https://github.com/lampepfl/dotty/pull/7427/files#diff-8865435e54a057041718570580f5d24fR544-R552
When unpickling Scala 2 trees: https://github.com/lampepfl/dotty/pull/7427/files#diff-59330d0079b0029a4e62c5d5d0385db2R194-R231

Also, the code used in hotspots is too expensive.

Is there anything else apart from the lazy vals that box and the ConditionallyImmutableFlags scaffolding?

I believe this suffers from too early generalization. It seems to the problem is only with Deferred, right? What exactly is the problem? Is there a cheaper way to solve it, without creating an expensive general scaffolding?

The problem with the Deferred is that the mechanism of the opaque types sets this flag for opaque types on their completion. Hence we cannot assume that Deferred is a FromStartFlag. However, if we just drop this assumption, things go wrong on unpickling at this line:

https://github.com/lampepfl/dotty/blob/d2db509f7b47c16edcad7d0772c58826c6cde897/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala#L675

So this line queries the flags of some class member sym while unpickling that class - !sym.isOneOf(DeferredOrLazyOrMethod)). If we try to complete the sym at that place, we'll get an exception like no symbol at Addr(23) – because the parent class was not fully unpickled when we tried unpickling its member symbol. However, if we cannot assume Deferred is immutable during completion, we have no other choice but to complete sym on that call – otherwise we cannot guarantee the symbol's Deferred status won't change.

So Deferred cannot be assumed immutable on completion but this assumption is critical for the correct unpickling order.

At the same time, at that particular place we can be absolutely sure Deferred will not change after completion. This is because isOneOf is evaluated only if sym.isTerm is true (the first operand to the &&). And we know that Deferred is mutated only for opaque types, which are types and not terms.

Hence the solution of the conditionally immutable flags.

Is there a cheaper way to solve it, without creating an expensive general scaffolding?

We can just hard-code the Deferred flag in the isCurrent method.

odersky · 2019-10-26T16:28:26Z

Can we set Deferred for opaque types from the start? If not, then the next best choice is to hardcode Deferred in isCurrent.

Is there anything else apart from the lazy vals that box and the ConditionallyImmutableFlags scaffolding?

I believe so. A reasonable way to track this is to count how many object allocations are executed by the code (lambdas count). For hot code the goal is 0.

If we are on completion but loading the symbol, allow modification of all the flags

Why do we need this?

anatoliykmetyuk · 2019-10-26T17:30:19Z

Why do we need this?

I believe because we are creating a symbol from scratch, so we should be able to modify anything at all about it.

anatoliykmetyuk · 2019-10-28T14:06:37Z

test performance please

dottybot · 2019-10-28T14:07:26Z

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

dottybot · 2019-10-28T17:47:13Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (9193c44)

…types Also fix a neg test for i3067. After this modification to the completion algorithm, implicit definitions without explicit types stay visible in the implicit scope and hence, the error of the missing implicit is not reporter. This is fine, however, since the real error to be fixed is the lack of an explicit type of the implicit value, not the lack of the implicit value itself.

This flag is set on symbol completion based on other semantic info. Fix i7407 test error message FTTB it is a cyclic error. It requests the user to fully specify the type of the method. Once they do so, an intended "implicit not found" message appears. Displaying the correct message directly requires adding `Macro` flag to `FromStartFlags`, which is not correct. Until we find a workaround, this solution is a reasonable trade-off because it offers an unambiguous action (to fully specify the type of the method) for the user to take to see the correct error message. The reason the cyclic error appears in first place is as follows. Given `inline def g = qc`, we typecheck `qc`. However, `qc` requires an implicit QuoteContext which can only be synthetically generated for the bodies of macros. Hence we need to see if `g` is a macro, and to do so, we need to complete it. To complete it, we need to typecheck its body, which yields the cyclic error.

Scala2x is a flag set for all the Scala 2 compliled classes. The information on what Scala version was used to compile the class file is available from the bytes of the classfile itself. Hence we cannot have this information from start, it will only be available on load. See ClassfileParser.unpickleOrParseInnerClasses for details on how the Scala version is determined.

This flag can be unpickled e.g. by Scala2Unpickler.readDisambiguatedSymbol. Hence it is not known from start.

`moduleRoot` is derived from moduleClassRoot which is being loaded. Since it is derived during the loading sequence of `moduleClassRoot`, we can argue that `moduleRoot` is loaded during this sequence as well. Hence we mark `moduleRoot` as loading by setting its completer to a temporary value. We set the completer back after the loading is done.

It must be set for derived value classes on completion.

…ssary

Make sure that we complete denotations only in case we absolutely must to when evaluating predicates on flags.

anatoliykmetyuk force-pushed the check-flags-on-completion branch 3 times, most recently from 12be72d to 56e8dfc Compare October 21, 2019 14:56

anatoliykmetyuk force-pushed the check-flags-on-completion branch 4 times, most recently from 44021ea to d8a51f2 Compare October 24, 2019 13:12

anatoliykmetyuk force-pushed the check-flags-on-completion branch from f7c1bb0 to aebd4c5 Compare October 25, 2019 12:23

anatoliykmetyuk mentioned this pull request Oct 25, 2019

Cosmetic changes to the completions framework #7451

Closed

anatoliykmetyuk marked this pull request as ready for review October 25, 2019 15:27

anatoliykmetyuk requested a review from odersky October 25, 2019 15:28

anatoliykmetyuk assigned odersky Oct 25, 2019

odersky requested changes Oct 26, 2019

View reviewed changes

odersky assigned anatoliykmetyuk and unassigned odersky Oct 26, 2019

anatoliykmetyuk force-pushed the check-flags-on-completion branch 2 times, most recently from 8d256c1 to 326696f Compare October 28, 2019 13:29

anatoliykmetyuk added 13 commits November 1, 2019 16:20

Add a setter for myFlags

928adf3

Add immutableFlags abstraction to SymDenotations

8148171

Add an immutable flags check to myFlags setter

6e350cf

Remove Touched flag from FromStartFlags since it is an internal flag

d481f91

Synthetic is an AfterLoadFlag

e1f1e61

This flag can be unpickled e.g. by Scala2Unpickler.readDisambiguatedSymbol. Hence it is not known from start.

Final flag is not AfterLoadFlag

da09201

It must be set for derived value classes on completion.

TreeUnpickler's completer is a LoadingCompleter

4506028

butNot-capable predicates on flags complete only when absolutely nece…

44ed627

…ssary

Lazy strategy for evaluating flags on denotations

d7a049b

Make sure that we complete denotations only in case we absolutely must to when evaluating predicates on flags.

anatoliykmetyuk force-pushed the check-flags-on-completion branch from 326696f to d7a049b Compare November 1, 2019 15:21

anatoliykmetyuk mentioned this pull request Nov 12, 2019

Remove Touched from FromStartFlags #7484

Closed

odersky mentioned this pull request Nov 14, 2019

Dotty with explicit nulls (and flow typing) #7546

Merged

anatoliykmetyuk closed this Feb 18, 2020

Check flags that are not supposed to be mutated by completion #7427

Check flags that are not supposed to be mutated by completion #7427

Uh oh!

Conversation

anatoliykmetyuk commented Oct 15, 2019

Uh oh!

smarter commented Oct 15, 2019

Uh oh!

anatoliykmetyuk commented Oct 21, 2019

Uh oh!

anatoliykmetyuk commented Oct 24, 2019

Uh oh!

anatoliykmetyuk commented Oct 24, 2019

Uh oh!

liufengyun commented Oct 24, 2019

Uh oh!

dottybot commented Oct 24, 2019

Uh oh!

anatoliykmetyuk commented Oct 24, 2019

Uh oh!

liufengyun commented Oct 24, 2019

Uh oh!

dottybot commented Oct 24, 2019

Uh oh!

anatoliykmetyuk commented Oct 25, 2019

Uh oh!

dottybot commented Oct 25, 2019

Uh oh!

dottybot commented Oct 25, 2019

Uh oh!

odersky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky Oct 26, 2019

Choose a reason for hiding this comment

Uh oh!

anatoliykmetyuk commented Oct 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky commented Oct 26, 2019

Uh oh!

anatoliykmetyuk commented Oct 26, 2019

Uh oh!

anatoliykmetyuk commented Oct 28, 2019

Uh oh!

dottybot commented Oct 28, 2019

Uh oh!

dottybot commented Oct 28, 2019

Uh oh!

Uh oh!

odersky left a comment •

edited

Loading

anatoliykmetyuk commented Oct 26, 2019 •

edited

Loading