Fix error initializeEnumMap() is exceeding the 65535 bytes limit #9954
Conversation
There was a problem hiding this comment.
Thanks for the contribution - keep in mind that this is going to eventually fail as well, as there is an upper limit to the size of a class file too. I don't have a concrete suggestion here, but it could be worth considering while you're making the change.
It might be worth it to change the implementation to not produce a Arrays.asList(MyEnum.FIRST, MyEnum.SECOND, MyEnum.THIRD) etc, but instead to use Arrays.asList(MyEnum.values()) - that would also allow the implementation to scale with the number of enum types rather than with the total number of enum values in all types.
Worth making either change to further improve this?
| sw.indent(); | ||
| for (Map.Entry<JEnumConstant, String> entry : model.getEnumTokenMap().entrySet()) { | ||
| // enumToStringMap.put(Enum.FOO, "FOO"); | ||
| sw.println("enumToStringMap.put(%s.%s, \"%s\");", entry.getKey().getEnclosingType() | ||
| .getQualifiedSourceName(), entry.getKey().getName(), entry.getValue()); | ||
|
|
||
| enumPerMethodCount++; | ||
| if(enumPerMethodCount >= maxEnumsPerMethod) { |
There was a problem hiding this comment.
checkstyle failure (this line and others below):
/home/runner/work/gwt/gwt/gwt/user/src/com/google/web/bindery/autobean/gwt/rebind/AutoBeanFactoryGenerator.java:386:7:
error: 'if' is not followed by whitespace. (com.puppycrawl.tools.checkstyle.checks.whitespace.WhitespaceAroundCheck)
/home/runner/work/gwt/gwt/gwt/user/src/com/google/web/bindery/autobean/gwt/rebind/AutoBeanFactoryGenerator.java:422:7:
error: 'if' is not followed by whitespace. (com.puppycrawl.tools.checkstyle.checks.whitespace.WhitespaceAroundCheck)
/home/runner/work/gwt/gwt/gwt/user/src/com/google/web/bindery/autobean/gwt/rebind/AutoBeanFactoryGenerator.java:437:5:
warning: '{' at column 5 should be on the previous line. (com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck)
It's not infinite but trying to figure out what the limit the upper limit on a class file is, it looks like the next limitation we'd run into having 65535 methods in one class. That's very far away for our use case as we're on only on the 3rd To your second question, I can't tell where you're talking about can you point it out please. Did you mean the line:
This line is generating the string mapping for enums that share the same name, so is generating something like |
Ah you're right of course, thank you for clarifying what I didn't read correctly. |
| sw.println("@Override protected void initializeEnumMap() {"); | ||
| int methodCount = 0; | ||
| int enumPerMethodCount = 0; | ||
| int maxEnumsPerMethod = 2000; |
There was a problem hiding this comment.
Maybe pull this up to a constant in the class? Also just double checking - this isn't counting the number of enums referenced in a single step through the second loop, so it is possible to get a single (or several) very large Arrays.asList(.....) calls that bloat a method beyond the tipping point. I think the value you've chosen here should make that unlikely, but just in case you wanted to count each individual value (with a enumPerMethodCount += entry.getValue().size() or the like) to be safe?
There was a problem hiding this comment.
Great suggestion! I changed it to enumPerMethodCount += entry.getValue().size() in the second loop. And pulled up the constant.
| methodCount++; | ||
| sw.println("private void initializeEnumMap_%d() {", methodCount); | ||
| sw.indent(); | ||
| } |
There was a problem hiding this comment.
The duplication in these two sections bothers me... but I think not enough to ask for it to be factored out into a lambda or class that keeps the code more readable.
(Unless you were looking for a nudge to clean that up further.)
Pre-submit edit: Okay two more things that bother me here (but again not enough to require a change before merge):
- the first initializedEnumMap_N call always has a zero suffix, but still uses
%d, which is a little silly. - It is possible to emit a new method at the end of a loop, thus leaving it empty.
It could make slightly more sense to remove the first method creation call, and move each "do i need to start a new method" check to the top of the loops they belong to. Then, initialize enumPerMethodCount to greater than maxEnumsPerMethod and you'll always get an method created if needed. Last, the final outdent+println(}) can be conditional - don't print if methodCount==0.
Thoughts?
There was a problem hiding this comment.
You are correct it could have created a new method at the end of a loop, thus leaving it empty. I moved the closing of the previous method and creation of a new method to the top of the loops.
One quick note the initializedEnumMap_N now start at 1 so the last loop to call the initializeEnumMap() now starts at 1 too.
There was a problem hiding this comment.
Changes look good, stylecheck looks good, but I see that this is rebased back to when we still used gerrit - any chance you should rebase/merge up to date locally for testing before we land this?
Or were you hoping to backport this to a release other than main (aka the upcoming 2.12 release)?
|
I merged locally to main and ran AutoBeanSuite, RequestFactorySuite, and RequestFactoryEditorSuite successfully, but would like to be sure that local builds merged up to date worked for you @pmskintner. |
…enum-map local testing
|
I merged gwt main into our fork locally and ran the test suites you mentioned, here are the reports: TEST-com.google.web.bindery.requestfactory.gwt.RequestFactoryEditorSuite.txt The upcoming 2.12 release would be great to get into, is that going to be tagged off the current main? |
|
Yes, that will be cut from main, once we have the other Java 17 features in. Can you merge this up to date with main for a quick round of your own testing of the feature, just to be sure, rather than rely on git doing it for us? |
|
We merged main and validated locally. |
During build we hit a the error
This gets around the error by splitting the method into as many methods as needed.