Increase ATN states size limit, simplify ATN serialization

[KvanTTT]

I'm returning to the increasing of ATN states size and I've also simplified serialization (related to Unicode encoding).

ATN states size can be > 65535, up to 2^31-1:

• fixes Add a more understandable message than "Serialized ATN data element .... element ... out of range 0..65535" #1863
• fixes UnsupportedOperationException while generating code for large grammars. #2732
• fixes Serialized ATN data element 810567 element 11 out of range 0..65535 #3338

Take a look at C# code: it contains small changes because it already has Read and Write methods (as well as other runtimes except for Java).

If Java and C# are ok, I'll complete other runtimes.

Writing, Reading methods have comprehensive tests: testATNDataWriterReaderCompact, testATNDataWriterReaderRaw.

[parrt]

Can you describe what needs to change to handle 32-bit ints using 16-bit unicode? I assume this is easy to fix for non-Java. Just use int not short. What is effect on Java ATN code size? Ah. I see:

runtime/Java/src/org/antlr/v4/runtime/atn/ATNDataWriter.java

Basically using high bits to encode right? Hmm...again I'm worried about a very rare cases causes a big code change. Not that this approach is "wrong" but always gotta worry about breaking stuff. Also wouldn't this hurt the common case to use upper bit(s)?

[KvanTTT]

Can you describe what needs to change to handle 32-bit ints using 16-bit Unicode?

We used a different encoding for 16-bit and 32-bit values:

antlr4/runtime/Java/src/org/antlr/v4/runtime/atn/ATNSerializer.java

Lines 166 to 201 in db8a483

	List<IntervalSet> bmpSets = new ArrayList<>();
	List<IntervalSet> smpSets = new ArrayList<>();
	for (IntervalSet set : sets.keySet()) {
	if (!set.isNil() && set.getMaxElement() <= Character.MAX_VALUE) {
	bmpSets.add(set);
	}
	else {
	smpSets.add(set);
	}
	}
	serializeSets(
	data,
	bmpSets,
	new CodePointSerializer() {
	@Override
	public void serializeCodePoint(IntegerList data, int cp) {
	data.add(cp);
	}
	});
	serializeSets(
	data,
	smpSets,
	new CodePointSerializer() {
	@Override
	public void serializeCodePoint(IntegerList data, int cp) {
	serializeInt(data, cp);
	}
	});
	Map<IntervalSet, Integer> setIndices = new HashMap<>();
	int setIndex = 0;
	for (IntervalSet bmpSet : bmpSets) {
	setIndices.put(bmpSet, setIndex++);
	}
	for (IntervalSet smpSet : smpSets) {
	setIndices.put(smpSet, setIndex++);
	}

Now the encoding is universal.

Basically using high bits to encode right?

Yes, two high bits are used for service purposes.

Also wouldn't this hurt the common case to use upper bit(s)?

All big Unicode values are encoded using not more than 2 integers in tests as it was before, I've checked: https://github.com/antlr/antlr4/pull/3546/files#diff-45a1efdccdd2d1ab783a98c0d1dc54c09914461d0c4bb0237d9dbe17bd4168dcR39

3 bytes are used for very big values (>= 2^31) and for negative numbers except -1 (negative numbers aren't used for serialization at all):

encoding	count	type
00xx xxxx xxxx xxxx	1	int (14 bit)
01xx xxxx xxxx xxxx xxxx xxxx xxxx xxxx	2	int (30 bit)
1000 0000 0000 0000 xxxx xxxx xxxx xxxx xxxx xxxx xxxx xxxx	3	int (32 bit)
1111 1111 1111 1111	1	-1 (0xFFFF)

Hmm...again I'm worried about a very rare cases causes a big code change. Not that this approach is "wrong" but always gotta worry about breaking stuff.

All values within the range Integer.MIN_VALUE..Integer.MAX_VALUE are covered by tests https://github.com/antlr/antlr4/pull/3546/files#diff-3ac04ac7612676174d4634b5bea3e2ef612af5a06f6887090e36450067be8d6fR163-R207:

@Test public void testATNDataWriterReaderCompact() {
	IntegerList integerList = new IntegerList();
	ATNDataWriter writer = new ATNDataWriter(integerList, "Java");
	assertEquals(1, writer.write(0));
	assertEquals(1, writer.write(-1));
	assertEquals(1, writer.write(42));
	assertEquals(2, writer.write(1 << 14));
	assertEquals(2, writer.write(0xFFFF));
	assertEquals(3, writer.write(Integer.MAX_VALUE));
	assertEquals(3, writer.write(Integer.MIN_VALUE));
	assertEquals(13, integerList.size());
	char[] charArray = Utils.toCharArray(integerList);
	ATNDataReader reader = new ATNDataReader(charArray);
	assertEquals(0, reader.read());
	assertEquals(-1, reader.read());
	assertEquals(42, reader.read());
	assertEquals(1 << 14, reader.read());
	assertEquals(0xFFFF, reader.read());
	assertEquals(Integer.MAX_VALUE, reader.read());
	assertEquals(Integer.MIN_VALUE, reader.read());
}
@Test public void testATNDataWriterReaderRaw() {
	IntegerList integerList = new IntegerList();
	ATNDataWriter writer = new ATNDataWriter(integerList, "Java");
	writer.writeInt32(0);
	writer.writeInt32(-1);
	writer.writeInt32(42);
	writer.writeInt32(1 << 14);
	writer.writeInt32(0xFFFF);
	writer.writeInt32(Integer.MAX_VALUE);
	writer.writeInt32(Integer.MIN_VALUE);
	assertEquals(7 * 2, integerList.size());
	char[] charArray = Utils.toCharArray(integerList);
	ATNDataReader reader = new ATNDataReader(charArray);
	assertEquals(0, reader.readInt32());
	assertEquals(-1, reader.readInt32());
	assertEquals(42, reader.readInt32());
	assertEquals(1 << 14, reader.readInt32());
	assertEquals(0xFFFF, reader.readInt32());
	assertEquals(Integer.MAX_VALUE, reader.readInt32());
	assertEquals(Integer.MIN_VALUE, reader.readInt32());
}

The ATN serializer format is already not back-compatible with the previous versions (because of UUID removing and version changing).

[parrt]

Hmm... I think @ericvergnaud had a concern about double encoding; first we encode to get larger integers and then we have to use modified UTF-8. in principle that's okay but it makes me nervous. I think Eric's concern was about all of the decoding on a phone.

Also, 14 bits only gives us 16384 max, which I think is hitting the size of the lexer for SQL grammars. I don't remember where we put the numbers I had but isn't it going to hurt grammar size for such large but 16-bit conforming grammars?

I see that we allow 32 bit values and sets but I don't see how our current code allows 32-bit single edge transitions in the ATN. do you have any idea if that is the case?

[KvanTTT]

Hmm... I think @ericvergnaud had a concern about double encoding; first we encode to get larger integers and then we have to use modified UTF-8. in principle that's okay but it makes me nervous. I think Eric's concern was about all of the decoding on a phone.

In my changes, there is no need to encode small numbers at first and big numbers at second, natural order is preserved. It should not break decoding on a phone.

Also, it looks like such Unicode encoding was introduced by @bhamiltoncx in fd4246c

Also, 14 bits only gives us 16384 max, which I think is hitting the size of the lexer for SQL grammars. I don't remember where we put the numbers I had but isn't it going to hurt grammar size for such large but 16-bit conforming grammars?

It does not hit the size of the lexer for SQL grammars because the max value is 13163 there: #3505 (comment) Anyway, SQL lexer is very big, other grammars are almost always smaller.

I see that we allow 32 bit values and sets but I don't see how our current code allows 32-bit single edge transitions in the ATN. do you have any idea if that is the case?

I've added the generated test getAtnStatesSizeMoreThan65535Descriptor that fails on the previous version. Actually, max is 31-bit transitions because of int is signed type.

[parrt]

thanks for the link back to the empirical tests I did for the SQL grammar.

Anyway, SQL lexer is very big, other grammars are almost always smaller

If this is true, seems like we are doing some major surgery to support a tiny subset of the population. on the other hand, it sounds like there are a number of people that are finding this useful; you passed some links to issues.

we could minimize the size of this change by limiting it to only those targets that need to use strings to encode integer lists, right? certainly that his java. can C# do static integer arrays properly? if so, then we don't need this for C# either. we should definitely not do this for any targets such as C++ that can directly encode static arrays. does JavaScript use strings? Dang, yep, looks like JavaScript uses strings as well:

const serializedATN = ["<model.serialized; wrap={",<\n>    "}>"].join("");

it looks like Java, C#, JS, Dart, Php, Python2/3, Swift. So, everybody except Go and C++? Dang. we should definitely get away from strings for any language that allows it. certainly this should be avoided for Python as it can simply do atn = [1,2,3,4...] right?

[parrt]

Ok, I've spent a lot of time thinking about this today and went back over the arguments from before. basically I'd like to stick with my original suggestion:

In other words, we begin the process by serializing an ATN into a list of integers. Then, we figure out the maximum value and see if everything fits in 16 bits. If so, we leave everything as is, otherwise we convert ALL int values to two \uXXXX chars rather than a single char.

It seems to me that we should simply get an IntegerList out of serialize() with the version number first and then the word size (16 or 32) with all words 16 or 32, no bit twiddling. The version number plus the word size indicates the encoding for words. (We can add a method to IntegerList to compute the maximum value.) In the future, it's possible I'd consider the optimization you have here, but I'd rather not do bit twiddling across all targets in one big move. The targets can do whatever they want with this information, generating strings or simply creating static short or int lists like C++ and Go. any target that is allowed to generate static integers should do so.

Sounds like we still need to convert the Java ATNDeserializer to use a "reader", as you have suggested, so that it can read either 16 or 32 depending on what kind of data we have stored. Others seem to use something similar but for example Python directly refers to readInt vs readInt32. We'll need a generic "read" that pulls 16 or 32 bits depending on the value following the version number.

[KvanTTT]

How to represent -1 value in 16-bit and 32-bit modes, 0xFFFF and 0xFFFFFFFF? I am afraid the current serialization works incorrectly if positive value 0xFFFF conflicts with the special value reserved for -1.

We'll need a generic "read" that pulls 16 or 32 bits depending on the value following the version number.

It also requires "bit twiddling" if 32-bit integer mode is activated. It's simple but it exists:

public int read() {
	return data[p++] | (data[p++] << 16);
}

[parrt]

Yup, sounds like we need to limit to 0xFFFF - 1 for valid 16 bit values.

You're right shifting by 16 is twiddling :) but simple enough I think.

[KvanTTT]

Yup, sounds like we need to limit to 0xFFFF - 1 for valid 16 bit values.

It's quite a strange solution, especially considering ANTLR is also a binary parser and 0xFFFF is a completely correct token type. I've created an issue for this: #3555

It seems to me that we should simply get an IntegerList out of serialize() with the version number first and then the word size (16 or 32) with all words 16 or 32, no bit twiddling. The version number plus the word size indicates the encoding for words. (We can add a method to IntegerList to compute the maximum value.)

I've tried to implement this idea but encountered the following problems:

• First two integers of ATN always should take 1 element instead of 2 (to preserve backward compatibility for version field). It complicates conversion to char arrays.
• It's unclear how to store -1 value in 16-bit and 32-bit modes, 0xFFFF and 0xFFFFFFFF? It is ugly and actually incorrect because of the abovementioned issue with the FFFF token type. Also, Java runtime requires -2 shifting.
• It generates bigger ATNs. Plain Int arrays take twice more memory.
• It complicates the serialization format because the second field with int size (16 or 32) should be filled after completion of the whole serialization step. Also, functionality with detection of max value should be integrated into IntegerList.

Ok, let's postpone fixing mentioned bugs because I don't think implementing such encoding is a very good idea. Actually, I think using IntegerList for serialization is not very good at all (specifically, restricting integers within 0..0xFFFF range), ByteList is preferable and it's a standard way in most binary serializers. I've described binary encoding using byte array in #3494

But feel free to use my commit with the writer/reader and implement your solution if you think it's correct.

[parrt]

Ok, sounds good. Valid points you raise but we can discuss in future. Thanks for all your efforts.