Closed Bug 625600 Opened 10 years ago Closed 10 years ago

Update Yarr import


(Core :: JavaScript Engine, defect)

Not set





(Reporter: dmandelin, Assigned: dmandelin)


(Blocks 1 open bug)


(Whiteboard: fixed-in-tracemonkey wanted-standalone-js)


(9 files, 8 obsolete files)

1.20 MB, patch
: review+
Details | Diff | Splinter Review
13.92 KB, patch
: review+
Details | Diff | Splinter Review
4.73 KB, patch
: review+
Details | Diff | Splinter Review
4.46 KB, patch
: review+
Details | Diff | Splinter Review
5.88 KB, patch
: review+
Details | Diff | Splinter Review
284.81 KB, patch
: review+
Details | Diff | Splinter Review
135.08 KB, patch
: review+
Details | Diff | Splinter Review
1.51 MB, patch
Details | Diff | Splinter Review
231.68 KB, application/x-zip-compressed
I don't think this should block for now; we should only block on things that are proven to affect the web. But it's fairly important and we should do as much of this as time permits before Fx4. We should probably start by surveying all the changes that landed since Chris's last import, seeing if they fix things that look important and if they caused regressions, and go from there.
No longer blocks: 625502
Preliminary report:

Currently, our import of yarr is:

       rev 68100
       disable the yarr interpreter
       plus cherry-picked rev 72781

The revs that change yarr that we have not taken are:

* Fixes

72813 + 73594 - don't match bogus char class + back off a bit for web compat
73617 - something with backtracking
74309 - crash on Yelp
75408 - make syntax errors early errors
75796 - backtracking for nested alternatives
75991 - fuzzer crash
76275 - crash
76407 - hang

* Regression Fixes

76076 + 76133 (followup for regression introduced by 76076)

* Refactorings

72999 - typos
[[75595 + 75597]] (backed-out patch)

* Optimizations

68207 - improve v8-regexp by 2.7%
74441 - reduce cache mem use

* Interpreter

69842 + 69847 (build fix for 69842)
[[72197 + 72781]] (backed-out patch)
I think the next step is to go through the fixes and regression fixes and see which we are currently affected by. Then we can try to import the patches that fix those. But we need to check for regressions from them, and see which additional patches we would need to import in order to import those patches.
For all the fixes (not including regression fixes) that have obvious test cases, we currently pass those tests. So I think we are pretty good here and don't need to do anything else before Fx4.
Blocks: 638208
Assignee: general → dmandelin
I'm working on this now. It's probably going to take a week or two, partly because it's been a few months since we've updated so there's a lot to go through, and partly because I'll be gone for a day or two for a conference next week.
Blocks: 644143
Blocks: 633677
Blocks: 652909
Starting this up again. In order to make future merging easier and upstreaming possible, I want to make minimal changes to the WebKit code, and instead add a bridge that does things like implement the WebKit Vector interface on top of our implementation. Most of this should not be too hard, but there is a sticking point:

The JSC/Yarr code uses refcounting smart pointers quite a bit: a RefCounted template, RefPtr and PassRefPtr pointers, etc. Ideally, we would just provide our own implementation of enough of this stuff to make it work.

The problem is that the refcounting template code in WebKit is LGPL 2+, which, IIUC, is not compatible with the tri-license and cannot be imported into our tree. What can I do?

Can I reimplement that functionality myself? I have looked at those files a little bit, e.g., to understand what they are for and to check on their license.
I can confirm that LGPL2+ is not permitted by Mozilla licensing policy (and won't be even when/if we move to MPL 2).

Blocks: 654693
Attached patch WIP (obsolete) — Splinter Review
Builds on Windows but asserts if you try to run a regexp--lots of fixmes left. The good news is that I was able to get it to build making relatively few changes to the Yarr code. I think I will make Yarr use our hash tables, though--they only use a hash table in one place, and the API impedance mismatch is pretty bad.
Attached patch WIP 2 (obsolete) — Splinter Review
This works for the most part on Windows. There are some jit-test failures. One is bug 576837, which I had thought was fixed upstream, but apparently not. I can just take our fix and hopefully upstream that as well. There are some other failures that seem to work correctly in the latest WebKit, so I'll have to update to that. There are also about 20 other issues to check on, such as Mac/Linux testing and header file cleanups.
Attachment #532027 - Attachment is obsolete: true
Attached patch WIP 3 (obsolete) — Splinter Review
Passes jit-tests on Windows. Now I just need to crank through 20 or so engineering issues. The only hard one is memory management--it probably leaks right now and needs some replacement for the refcounting smart pointers.
Attachment #532278 - Attachment is obsolete: true
Attached patch WIP 4 (obsolete) — Splinter Review
What's left is to be fixed up for Mac and Linux, update license files, and fix memory managements.
Attachment #532352 - Attachment is obsolete: true
Attached patch WIP 5 (obsolete) — Splinter Review
Almost done. I just need to check on Linux compat.
Attachment #532379 - Attachment is obsolete: true
Attached patch WIP 6 (obsolete) — Splinter Review
I found a few bugs with jstests, so I had to refresh Yarr, readapt that (which turned out to be very easy with my approach of keeping a separate "adapt Yarr to Moz" patch), and fix another bug that cdleary might have fixed earlier, or else WebKit decided to unfix.

This still needs Linux/Mac testing, but hopefully is really really close now.
Attachment #532764 - Attachment is obsolete: true
I'm going to put this up as a series of patches, so that each part of the adaptation is documented. This should also make it easier to review.

Question about landing, though: Do you think I should land it in parts, so that it is permanently documented in the tree, or as one patch, so that it's easier to back out later if that's required?
Attachment #532816 - Attachment is obsolete: true
Attachment #532830 - Flags: review?(cdleary)
Attachment #532831 - Flags: review?(cdleary)
Attachment #532832 - Flags: review?(cdleary)
I kept this separate because it's more likely to change than the rest.
Attachment #532833 - Flags: review?(cdleary)
This bug was latent in Yarr rev 86639. It is not present in our initial import; I don't know if cdleary fixed it or if Yarr had it right at that point.
Attachment #532834 - Flags: review?(cdleary)
cdleary fixed this in the original import; this is just an update of his fix. Yarr previously fixed this bug, but they fixed it too hard (too restrictive), so they undid the fix.
Attachment #532835 - Flags: review?(cdleary)
This is optional; it's not needed to fix this bug correctly. We had previously added 'isValid' methods to some Nitro asm jump holder classes. Since then, they have added 'isSet'. This just renames one of our 'isValid' to match their 'isSet', in a class that would otherwise need both APIs.
Attachment #532837 - Flags: review?(dvander)
Attachment #532831 - Attachment is obsolete: true
Attachment #532831 - Flags: review?(cdleary)
Attachment #532832 - Attachment is obsolete: true
Attachment #532841 - Flags: review?(cdleary)
Attachment #532832 - Flags: review?(cdleary)
Blocks: 605998
Attachment #532837 - Flags: review?(dvander) → review+
Comment on attachment 532830 [details] [diff] [review]
Part 1: Remove old yarr files

A few comments:

- I'm guessing that all of the FIXMEs are imported and left in for ease of merging from upstream.
- I would make sure this all builds with --disable-methodjit (I see you added the include of "methodjit/MethodJIT.h" in the inlines file).
- I'd prefer it if we could keep the android bug-reference comment -- ideally that EnableYarrJIT wouldn't be there at all.
- executeInternal has an awkwardly expressed JS_ASSERT(result >= 0) with JS_ASSERT(0)s in it.
- "const size_t notFound = static_cast<size_t>(-1);" Does the static cast buy you anything here? I think signed -1 should just signex to fill the size_t.
- In compileHelper you have to JS_ASSERT(0) if !ENABLE_YARR_JIT. Maybe this will be changed in the rest of the patches?

I'm not sure this is something we can commit piecemeal, since all the prior YARR files are ripped out. Making changesets which can compile and run the full gamut of tests is the rule of thumb I'm familiar with.
Attachment #532830 - Flags: review?(cdleary) → review+
Comment on attachment 532838 [details] [diff] [review]
Part 2 (fixed): Import Yarr rev 86639 unmodified

Not having reviewed all the constituent changesets on the webkit side, rs=me.
Attachment #532838 - Flags: review?(cdleary) → review+
Comment on attachment 532841 [details] [diff] [review]
Part 3 (fixed): Adapt Yarr rev 86639 to build in Spidermonkey

These char-error interfaces introduce some unfortunate merge-from-upstream issues -- maybe we can get those a little more loosely coupled upstream in the future.

One note: it looks like some of your Makefiles are replacing tabstops with 4 softabstop, in the prior patches as well, which I forgot to mention.
Attachment #532841 - Flags: review?(cdleary) → review+
Comment on attachment 532833 [details] [diff] [review]
Part 4: Fix Yarr memory management to work with SpiderMonkey

There's one "// YYY" comment that may need to be removed. When I first implemented the CharTable ownership there were a few tricky leaks -- this doesn't look different from the way it was, but if it happens to be or you're feeling extra cautious I would give the /regexp/i reftests a run through valgrind.
Attachment #532833 - Flags: review?(cdleary) → review+
Attachment #532834 - Flags: review?(cdleary) → review+
Attachment #532835 - Flags: review?(cdleary) → review+
Landed with a few nit-fixes added.
Backed out:

I think I bumped into the bug changing the new ctors. Will push a corrected version soon.
Rebacked out. I already bounced this off TM a few times today, thinking that maybe there was just a small issue or 2. But there is some jsvector re-entrance issue, and some ARM build stuff, that for some reason didn't pop on try. So I'm not sure if this will be able to land before tomorrow. I'm not sure that a 2MB that lands just before the branch would be likely to get through Aurora without getting backed out, anyway. But we'll see.
Depends on: 659210
Attached patch Updated versionSplinter Review
Takes care of issues introduced by bug 657384 and works correctly on no-jit builds. There are still some Android build issues to be sorted out.
This is a zip of the corrected patch queue I used to do this Yarr import. It has these patches, in this order:

      Deletes the old Yarr files, from the first import, to get a clean tree.

      Copies in Yarr files from the given WebKit rev, and nothing more.

      Adapts Yarr and SpiderMonkey to each other, except for memory management
      issues, because those are more likely to change.
      Adapt memory management.

      Fix a Yarr bug with detecting out-of-range numeric inputs.
      Fix a Yarr bug with being too permissive on character classes

      Refactor a bit of our imported Yarr assembler to more closely match WebKit.

The idea here is that the next refresh of Yarr rev R can be done like this:

1. Delete the current Yarr files. Name this patch del-yarr-rev-86639.diff.
2. Copy in Yarr files for rev R. Name this patch import-yarr-rev-R.diff.
3. Apply the rest of the patches in order, fixing them as necessary and updating.

I actually had to do this during my work on this bug, because I discovered a bug in Yarr that had been fixed a few days after my import. I found that the patch queue applied pretty cleanly.
Depends on: 660371
Depends on: 660591
No longer depends on: 660371
Depends on: 661973
Duplicate of this bug: 657804
Duplicate of this bug: 654693
Duplicate of this bug: 652909
Duplicate of this bug: 644143
Duplicate of this bug: 633677
Duplicate of this bug: 664527
Duplicate of this bug: 670031
Depends on: 670003
Where is this fixed? Please set status flags and Target Milestone
Depends on: 679986
Depends on: 682252
Depends on: 682572
Depends on: 683838
Depends on: 679936
Target Milestone: --- → mozilla8
Depends on: 685477
Depends on: 687302
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey wanted-standalone-js
Depends on: 691172
Depends on: 751124
Depends on: 756838
Depends on: 885299
Depends on: 898189
Depends on: 938020
You need to log in before you can comment on or make changes to this bug.