Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix error initializeEnumMap() is exceeding the 65535 bytes limit #9954

Merged
merged 4 commits into from
Jul 16, 2024
Merged

Fix error initializeEnumMap() is exceeding the 65535 bytes limit #9954

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2fd2833
split initializeEnumMap into multiple methods
pmskintner May 8, 2024
88cbe3c
checkstyle fixes
pmskintner May 15, 2024
f41414e
Start loops with method creation
pmskintner May 20, 2024
d18a741
Merge remote-tracking branch 'upstream/main' into split-initiliazing-…
pmskintner May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion user/src/com/google/web/bindery/autobean/gwt/rebind/AutoBeanFactoryGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,12 +372,25 @@ private void writeEnumSetup(SourceWriter sw) {
list.add(entry.getKey());
}

sw.println("@Override protected void initializeEnumMap() {");
int methodCount = 0;
int enumPerMethodCount = 0;
int maxEnumsPerMethod = 2000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion! I changed it to enumPerMethodCount += entry.getValue().size() in the second loop. And pulled up the constant.

sw.println("private void initializeEnumMap_%d() {", methodCount);
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) {
Copy link
Contributor

@niloc132 niloc132 May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

sw.outdent();
sw.println("}");
enumPerMethodCount = 0;
methodCount++;
sw.println("private void initializeEnumMap_%d() {", methodCount);
sw.indent();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
for (Map.Entry<String, List<JEnumConstant>> entry : map.entrySet()) {
String listExpr;
Expand Down Expand Up @@ -405,6 +418,24 @@ private void writeEnumSetup(SourceWriter sw) {
listExpr = sb.toString();
}
sw.println("stringsToEnumsMap.put(\"%s\", %s);", entry.getKey(), listExpr);
enumPerMethodCount++;
if(enumPerMethodCount >= maxEnumsPerMethod) {
sw.outdent();
sw.println("}");
enumPerMethodCount = 0;
methodCount++;
sw.println("private void initializeEnumMap_%d() {", methodCount);
sw.indent();
}
}
sw.outdent();
sw.println("}");

sw.println("@Override protected void initializeEnumMap() {");
sw.indent();
for (int i = 0; i <= methodCount; i++)
{
sw.println("initializeEnumMap_%d();", i);
}
sw.outdent();
sw.println("}");
Expand Down
Loading