Skip to content

FIx constant pool construction and throw correct exception on overflow. - #2498

Open
aardvark179 wants to merge 3 commits into
mozilla:masterfrom
aardvark179:aardvark179-const-pool-overflow-fix
Open

aardvark179 wants to merge 3 commits into
mozilla:masterfrom
aardvark179:aardvark179-const-pool-overflow-fix

Conversation

@aardvark179

Copy link
Copy Markdown
Contributor

@gbrail this should deal with the problems you saw related to #2461 and I was seeing on my const and loop fix branch. I've put it as a separate PR because I don't know what order you want to make these changes in.

@gbrail gbrail left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm happy to bring this in before the larger fix, but there are opportunities to make this change a bit more robust if we can.

}

void setConstantData(int index, Object data) {
if ((index & 0xffff) != index) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

My AI friend's is convinced that 65535 is not a valid index even though it would pass this test because of the way that we often compute or store itsTopIndex + 1. (And FWIW we seem to +2 it a lot as well.) That would imply a different check here. A unit test to verify the duplication or non-duplication here would probably answer for sure.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's also a fair point that we make this overflow check in setConstantData but not in other places that allocate handles, including addConstant and others.

setConstantData(itsTopIndex, r);
itsPoolTypes.put(itsTopIndex, CONSTANT_InterfaceMethodref);
index = itsTopIndex++;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is the goal here to put the calculated value of "index" back in the "itsConstantHash" table? It doesn't appear that we do.

@aardvark179
aardvark179 force-pushed the aardvark179-const-pool-overflow-fix branch from 0a6a640 to 8345119 Compare September 21, 2026 10:31
@aardvark179

Copy link
Copy Markdown
Contributor Author

Okay, I got my robot friend to generate a bunch of unit tests and then do a more extensive rewrite. I moved all the indices to be ints to avoid the types of conversion errors we have previously seen. I think there is much more comprehensive refactoring and API change lurking in there which might help significantly improve class file compilation speed (profiling suggests we spend a large amount of the time repeatedly building and finding things like TypeInfo structures), but that's not a change to do right now.

@aardvark179
aardvark179 force-pushed the aardvark179-const-pool-overflow-fix branch from 2e27c3d to 233cbd6 Compare September 24, 2026 16:05
@aardvark179

Copy link
Copy Markdown
Contributor Author

I've added some simple class splitting to handle large scripts and emit a size exception if we emit too many invokeDynamic instructions in a single class (which seems to be the cause of the three.js problems. It works on the spot level of scripts, and cannot split large single nodes, or individual large methods, so compilation can still fall back to the interpreter for some scripts. three.js now successfully compiles to class files in the benchmarks.

@aardvark179
aardvark179 force-pushed the aardvark179-const-pool-overflow-fix branch from 233cbd6 to a70c0d0 Compare September 29, 2026 09:47

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants