Use arrayview for bit set

[ericvergnaud]

Implements perf improvement by using array view for bit set

[ericvergnaud]

ignore the unrelated php failure

[kaby76]

I will be doing the review in three comments: "performance", "correctness", and "code issues".

Performance

This graph shows for java/java the runtimes for CSharp, Antlr4ng, TS 4.13.1 and TS with PR. There is about a 2.8176/2.7061 = 1.04 speed up (4%).

[ericvergnaud]

4% is great for such a small and low-risk change :-)

[kaby76]

All (271) grammars in grammars-v4 (except three, which have the "module not found" import issue uncorrected in base class code) pass using the runtime of this PR.

[kaby76]

Indent off.

[ericvergnaud]

Fixed

[kaby76]

Why are you constructing a new array when I think Uint32Array.resize() is available?

[ericvergnaud]

I'm afraid your thinking is incorrect. You can resize an ArrayBuffer but not a TypedArray. I suspect the reason for this is that the underlying buffer is shared (a poor design decision if you ask me) so resizing a view may actually silently resize other views. Resizing the buffer would be a hack, not something I'm comfortable with

[kaby76]

This code is definitely not correct:

antlr4/runtime/JavaScript/src/antlr4/misc/BitSet.js

Line 33 in e4a77df

this.data[index >>> 5] &= ~(1 << index);

You should not shift bit by index but by index mod 32.

See C# code.

antlr4/runtime/CSharp/src/Sharpen/BitSet.cs

Line 127 in 1b3150b

_data[element] &= ~(1UL << (index % BitsPerElement));

[ericvergnaud]

This code is definitely not correct:

antlr4/runtime/JavaScript/src/antlr4/misc/BitSet.js

Line 33 in e4a77df

this.data[index >>> 5] &= ~(1 << index);

You should not shift bit by index but by index mod 32.

See C# code.

antlr4/runtime/CSharp/src/Sharpen/BitSet.cs

Line 127 in 1b3150b

_data[element] &= ~(1UL << (index % BitsPerElement));

mmm... it("clears 1 value", () => { in BitSetSpec says it's ok... digging...

[kaby76]

antlr4/runtime/JavaScript/src/antlr4/misc/BitSet.js

Lines 31 to 33 in e4a77df

	const slot = index >>> 5;
	if (slot < this.data.length) {
	this.data[index >>> 5] &= ~(1 << index);

Let's consider some examples.

index = 0 => slot = index/32 = 0. 1<<(index%32) = 0x00000001
index = 1 => slot = index/32 = 0. 1<<(index%32) = 0x00000002
index = 31 => slot = 31/32 = 0. 1<<(index%32) = 0x80000000.
index = 32 => slot = 32/32 = 1. 1<<(index%32) = 1. data[0] = 0x00000000, data[1] = 0x00000001.

For index = 32, if you compute "1<<index" instead of "1<<(index%32)", the result is 0 (assuming 32-bit integer value of the shift). That's not correct.

Also "index >>> 5" is computed twice in the code.

[kaby76]

Actually, "1<<index" is probably ok because JS has a very very weird "<<" operator unlike every programming language in existence. 1<<67 = 8 in JS. But, that is the same as 1<<(67% 32). In other words, "1<<(index % 32)" is the same as "1<<index".

[ericvergnaud]

Actually, "1<<index" is probably ok because JS has a very very weird "<<" operator unlike every programming language in existence. 1<<67 = 8 in JS. But, that is the same as 1<<(67% 32). Very weird language. Still I would need to think about this.

I just checked manually, it's ok.

[ericvergnaud]

Also "index >>> 5" is computed twice in the code.

Fixed

[kaby76]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Left_shift

The right operand will be converted to an unsigned 32-bit integer and then taken modulo 32, so the actual shift offset will always be a positive integer between 0 and 31, inclusive. For example, 100 << 32 is the same as 100 << 0 (and produces 100) because 32 modulo 32 is 0.

[ericvergnaud]

If all ok with you can you approve ?