Open Bug 1367105 Opened 2 years ago Updated 5 days ago

Sync irregexp with upstream

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- affected

People

(Reporter: shu, Unassigned, NeedInfo)

References

(Blocks 4 open bugs)

Details

Attachments

(1 file)

370.92 KB, application/x-zip-compressed
Details
There are several stage 3 TC39 proposals regarding regexps in flight:

1. lookbehind (https://github.com/tc39/proposal-regexp-lookbehind)
2. Unicode groups (https://github.com/tc39/proposal-regexp-named-groups)
3. Unicode properties (https://github.com/tc39/proposal-regexp-unicode-property-escapes)
4. dotAll (https://github.com/tc39/proposal-regexp-dotall-flag)

These are already implemented in V8, so we should take them.
Blocks: 1361856
Blocks: 1361876
(In reply to André Bargull [:anba] from comment #1)
> http://searchfox.org/mozilla-central/source/js/src/tests/ecma_6/RegExp/
> unicode-ignoreCase.js needs much longer after updating to upstream irregexp,
> filed https://bugs.chromium.org/p/v8/issues/detail?id=6727 for further
> investigation.

Does this know you have a patch for updating irregexp? If so, can you post it here, even though it's not ready to land because of the perf regression? Or did you just test the performance in v8 and expect the same regression to happen in SM?
Flags: needinfo?(andrebargull)
(In reply to Till Schneidereit [:till] from comment #2)
> Does this know you have a patch for updating irregexp? If so, can you post
> it here, even though it's not ready to land because of the perf regression?
> Or did you just test the performance in v8 and expect the same regression to
> happen in SM?

Yes, I have a very raw patch (or rather queue of patches) to sync with upstream. Some of the patches still need to be reordered, or in some cases later patches contain changes which actually belong to an earlier patch. There are lots of changes, simply because
1. Upstream has some methods in other files, so I had to reorder many methods/classes to match upstream, which makes it easier to compare differences between our version and upstream.
2. Our irregexp version was partially reformatted using the SpiderMonkey coding styles, which made it quite difficult to spot changes when comparing to upstream irregexp, so I had to change it back to the upstream formatting.

I pushed the changes to try:
https://hg.mozilla.org/try/pushloghtml?changeset=958c5b935c048c2ee59d608ae304def15804a976

Things still missing in those patches:
1. Support for the stage 3 proposals (it's probably cleaner to implement those changes in separate bugs)
2. Updates for NativeRegExpMacroAssembler (sstangl already volunteered to port any necessary assembler changes :-)
Flags: needinfo?(andrebargull)
(In reply to André Bargull [:anba] from comment #3)
> Things still missing in those patches:
> 1. Support for the stage 3 proposals (it's probably cleaner to implement
> those changes in separate bugs)
Chrome 64 already enables all Stage 3 proposals by default, so maybe they are ready to be imported now.
André, sorry for not following up sooner, but can you update the patches to fix the -werr issue? Most of the new spec changes blocked by this have reached stage 4 today, so it'd be great if we could ship them soon-ish.
Flags: needinfo?(andrebargull)
Upstream landed some bug-fixes and performance improvements we definitely want to take to avoid regressions compared to our current irregexp version. On top of that, upstream landed a few clean-ups which "nicely" conflict with the current patches [1,2,3] and also other internal changes like [4] which we probably should take, too, so it's easier for us to implement the stage 4 proposals. I did start updating the patches this week, but it's a cumbersome process because I effectively need to compare every single irregexp file with a diff-tool to see what needs to be updated in our local copy. :-/

[1] https://github.com/v8/v8/commit/62f929ff4cf322bc05e3a384c4ad18005f50ebda
[2] https://github.com/v8/v8/commit/447af335e044c92196de3c3ddf1059fad6901650
[3] https://github.com/v8/v8/commit/ab43c76dde97188e79a6a1c5bb994407606521d2
[4] https://github.com/v8/v8/commit/04f7d484db22b1526afa5414c06eda443c5b4fad
Flags: needinfo?(andrebargull)
This issue blocks 4 RegExp features that ware already approved by TC39 and are now part of the ES2018 standard.
Priority: -- → P1
André, we have to get this done. Ashley's interested in finishing the work. Are the patches linked in comment 3 the latest version? Are those patches still a good starting point?
Flags: needinfo?(andrebargull)
I have local (git) patches from this year April, which are probably a better starting point, except I don't know offhand in which state these patches are in. I'll try to look into them in the next week. (Keeping the NI for now so I don't forgot about this.)
Attached file irregexp-patches.zip
My current patches, last updated in April.
Flags: needinfo?(andrebargull)
Flags: needinfo?(jorendorff)
Depends on: 1510128

I assume the patches are outdated again. What blocked you from looking into them?

Sebastian

Type: defect → enhancement
Flags: needinfo?(andrebargull)

(In reply to Sebastian Zartner [:sebo] from comment #11)

I assume the patches are outdated again. What blocked you from looking into them?

Other tasks which are more pleasantly to work at compared to this one. :-)

Flags: needinfo?(andrebargull)

Actually, the last code merge to irregexp was on nov/2017, so I believe this patch may still apply...

This is the only bug blocking the last missing 4 ECMAScript 2018 features. It would be nice to have it for Firefox 68, which is an ESR version.

Given it's a week til the branch-off for 68 and we're into a soft freeze and we don't have a full patch for this regardless, I'm pretty confident this isn't going to make 68, nice as it might be if it did.

How about 69?

We already have 70, so no.

Could we at least undo the formatting so that diffing can be easier?

Something needs to happen here, we can't just pretend this isn't important. Steven any ideas?

Flags: needinfo?(sdetar)
You need to log in before you can comment on or make changes to this bug.