Update to NET 8

[lahma]

• samples and tests run on NET 8
• packages updated

Like in Jint, it seems that NET 8 has degraded in inlining capabilities and we have to lower the check constant for time being.

[sebastienros]

it seems that NET 8 has degraded in inlining capabilities

I assume it's not easy to find out the scenario. Might be a common thing. Maybe could be detected by throwing random [MethodInlining] and see when that affects the results. That could be in private code blocks though too.

About the regex tests failing, maybe it's missing a font. ;)

[sebastienros]

But about the regex thing, I'd like to know what has changed exactly, I wonder which version is buggy. all .NET version before 8, or 8.

[lahma]

it seems that NET 8 has degraded in inlining capabilities

I assume it's not easy to find out the scenario. Might be a common thing. Maybe could be detected by throwing random [MethodInlining] and see when that affects the results. That could be in private code blocks though too.

I experimented a little and adding AggressiveOptimization allowed a bit larger threshold for tests, but the current defaults expected have somewhat changed. Some paths we could get away with source generators too (passing Func to use generic method for everything, could generate all cases without the need for Func).

But about the regex thing, I'd like to know what has changed exactly, I wonder which version is buggy. all .NET version before 8, or 8.

Tests were successful with NET 6 / FW 4.6.2, so I guess something is stricter / behaves differently with NET 8. Esprima rewrites them quite aggresively so I think @adams85 is our specialist for the matter in hand.

The TimeZone problem found in Jint already was classified as bug so I guess we should have just run previews more aggressively 😉

[lahma]

So now this subject matter expert just rides in with the white horse and makes me look just plain stupid, thanks for that 😉

[adams85]

But about the regex thing, I'd like to know what has changed exactly, I wonder which version is buggy. all .NET version before 8, or 8.

For rewriting unicode regexps, we rely on the CharUnicodeInfo.GetUnicodeCategory API. Apparently, MS updated the underlying Unicode dataset in .NET 8.

So, it is needed to regenerate the regexp testcases using the included generator.

Like in Jint, it seems that NET 8 has degraded in inlining capabilities and we have to lower the check constant for time being.

About this I have no clue at the moment. But a degradation of 450 -> 380 looks pretty rough...

[lahma]

About this I have no clue at the moment. But a degradation of 450 -> 380 looks pretty rough...

It is, I didn't want to touch it for now so improvements/changes are separate measurable PRs.

[lahma]

A humble thank you @adams85 , probably would have taken me ages to figure this out!

[adams85]

Sure thing!

BTW, there is a part which was pretty fragile in .NET 6: https://github.com/sebastienros/esprima-dotnet/blob/main/src/Esprima/Token.cs#L132-L141 So, I tweaked it a bit but it seems that not there lies the rub.

Couldn't this inlining degradation be related to the JIT improvements of .NET 8?

[sebastienros]

So, it is needed to regenerate the regexp testcases using the included generator

Two options:

• use a source generator so it's automatic
• add a nice comment in the unit tests to remind that if a test fail maybe the generator should be run again

[sebastienros]

Or generate the file as part of the build, and if the file is different than the checked-in one, fail the build.

[adams85]

A source generator or running the current generator as part of the build process is pretty much out of question because generating the test cases involves running scripts on Node.js. At least, I don't think it would be a great idea to make our build process depend on Node.js being installed.

Of course, we could add some checks to CI, however test cases will change pretty rarely, so it's not worth the hassle IMO. That leaves us with "add a nice comment in the unit tests to remind that if a test fail maybe the generator should be run again". Will do that soon.

[sebastienros]

Out of curiosity, are we using NodeJS because it has the correct RegExp we need, on just because it was easier for you to write it in JS? If the latter could we use Jint?
Another sub-comment, maybe NodeJS is already installed on the base image of the GH image, the same way net8.0 is installed by default now.

[adams85]

Out of curiosity, are we using NodeJS because it has the correct RegExp we need, on just because it was easier for you to write it in JS? If the latter could we use Jint?

It's the former. To be able to verify our output, we need standard-compliant reference data. We could use whatever well-tested JS engine for this purpose, I just happened to choose Node.js.