RFR: 8241356: Use a more reliable way to encode Symbol flags [v2]
Maurizio Cimadamore
mcimadamore at openjdk.java.net
Wed Feb 3 10:54:42 UTC 2021
On Tue, 2 Feb 2021 16:28:01 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:
>> [This is a GitHub copy of: https://mail.openjdk.java.net/pipermail/compiler-dev/2020-March/014389.html ]
>>
>> Currently, (com.sun.tools.javac.code.)Symbol/s have a long field "flags_field", which holds various one-bit information ("Flags") about the given Symbol.
>>
>> We currently have around 64 such Flags, which means we are out of bits in the long field, and adding new flags is not easy.
>>
>> We could change the "flags_field" to be a Set over enums, but this would increase the memory footprint notably, and would also slow-down access to flags. Currently, flags operations in javac are very fast and very common, so this is probably too much burden. There are also flags to which we need to access as bit masks, e.g. due to writing to classfile.
>>
>> My proposal here is to use an intermediate solution, until we find a better solution, or until a better solution is possible (like due to Valhalla). The idea is as follows:
>> -the current long-based Flags are split into 4 groups:
>> --"flat" Flags, long based, work exactly as before.
>> --enum-based TypeSymbolFlags, MethodSymbolFlags and VarSymbolFlags, which are only applicable to TypeSymbols, MethodSymbols and VarSymbols, respectively, and are checked using methods like Symbol.isFlagSet and set/clear using methods Symbol.setFlag and Symbol.clearFlag. So these flags are mostly encapsulated, even though physically they are currently stored in the flags_field as well.
>>
>> There are ~~37~~ 40 "flat" flags and ~~16~~ 17 TypeSymbolFlags (methods and vars have less flags), 57 in total. This gives us at least 7 new flags before we run out of long bits in flags_field again - but even if we do, there are several easy mitigation strategies we could use, like:
>> -create a new int/long field on TypeSymbols for the TypeSymbolFlags (probably preferable)
>> -split TypeSymbolFlags into Class/Package/MethodSymbolFlags
>>
>> The positives of this solution include:
>> -safe(r) access to the flags - the access to the extra/symbol-kind-specific flags is mostly encapsulated, and hence mostly safe
>> -improves the abstractions at least for some flags
>> -reasonably complex patch (attempts to encapsulate all the flags were troublesome in previous experiments)
>> -the performances appears to be acceptable.
>>
>> The negative that I see is that the code includes several incompatible types of flags now. These cannot be easily confused by accident (as the type system will complain), but still we need to be aware of them when writing code.
>>
>> Some more alternatives:
>> -add a new field to Symbol, like long extraFlags, and continue with bit masks as we did so far using this new field. The drawback is that it is more error prone (it would be difficult to ensure only the new/extra flags would be stored and read from extraFlags).
>> -have a new field, and "flat" and non-"flat" enum-based encapsulated Flags, but don't separate Type/Method/Var flags. Probably something to consider, even though I am a bit reluctant to add new fields without trying to avoid it .
>>
>> Any feedback is welcome!
>
> Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision:
>
> Removing merge tags are noted on the review - thanks!
Good experiment! I don't mind too much the fact that we have sets of incompatible flags - but while looking at the new Flags.java I noted several flags that looked *kind-specific* which seem to be defined in the *global* bucket. If (as I suspect) this is no accident, then I guess I'm a bit worried that the proposed patch would land us in a state where _some_ var flags are in the VarSymbolFlags enum, but not _all_ of them.
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Flags.java line 167:
> 165: /** Flag for synthesized default constructors of anonymous classes.
> 166: */
> 167: public static final int ANONCONSTR = 1<<27;
Question: why this, and other flags that are specific to only one symbol kind have not been translated into the enum? Other examples are `ANONCONSTR_BASED`, `GENERATEDCONSTR`, `COMPACT_RECORD_CONSTRUCTOR`, `THROWS` and probably more.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2316
More information about the compiler-dev
mailing list