On Tue, 2 Feb 2021 16:28:01 GMT, Jan Lahoda <jlah...@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
