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! I've reviewed the one javadoc change, and made a comment there. Although I approve the change to that one file, I will leave the review/approval of the javac changes to the javac experts. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Extern.java line 594: > 592: } else { > 593: ModuleSymbol msym = (ModuleSymbol) me; > 594: return msym.isFlagSet(TypeSymbolFlags.AUTOMATIC_MODULE); The change is OK, but the code being changed is not. There should not be references to any `javac.code.*` types outside of the `WorkArounds` class. Ideally, this function (`isAutomaticModule`) should be on the `Elements` class. ------------- PR: https://git.openjdk.java.net/jdk/pull/2316
