625600 - Update Yarr import

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

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.

Comment 1

[David Mandelin [:dmandelin]]

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

73640
73866
73962
76076 + 76133 (followup for regression introduced by 76076)

* Refactorings

72999 - typos
73124
74412
74600
75421
[[75595 + 75597]] (backed-out patch)
75602

* Optimizations

68207 - improve v8-regexp by 2.7%
72180
73307
73903
74441 - reduce cache mem use
76502

* Interpreter

68127
68771
69781
69842 + 69847 (build fix for 69842)
70764
72140
72186
[[72197 + 72781]] (backed-out patch)

Comment 2

[David Mandelin [:dmandelin]]

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.

Comment 3

[David Mandelin [:dmandelin]]

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.

Comment 4

[David Mandelin [: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.

Comment 5

[David Mandelin [:dmandelin]]

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.

Comment 6

[Gervase Markham [:gerv]]

I can confirm that LGPL2+ is not permitted by Mozilla licensing policy (and won't be even when/if we move to MPL 2).

Gerv

Comment 7

[David Mandelin [:dmandelin]]

Attachment: WIP

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.

Comment 8

[David Mandelin [:dmandelin]]

Attachment: WIP 2

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.

Comment 9

[David Mandelin [:dmandelin]]

Attachment: WIP 3

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.

Comment 10

[David Mandelin [:dmandelin]]

Attachment: WIP 4

What's left is to be fixed up for Mac and Linux, update license files, and fix memory managements.

Comment 11

[David Mandelin [:dmandelin]]

Attachment: WIP 5

Almost done. I just need to check on Linux compat.

Comment 12

[David Mandelin [:dmandelin]]

Attachment: WIP 6

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.

Comment 13

[David Mandelin [:dmandelin]]

Attachment: Part 1: Remove old yarr files

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?

Comment 14

[David Mandelin [:dmandelin]]

Attachment: Part 2: Import Yarr rev 86639 unmodified

Comment 15

[David Mandelin [:dmandelin]]

Attachment: Part 3: Adapt Yarr rev 86639 to build in Spidermonkey

Comment 16

[David Mandelin [:dmandelin]]

Attachment: Part 4: Fix Yarr memory management to work with SpiderMonkey

I kept this separate because it's more likely to change than the rest.

Comment 17

[David Mandelin [:dmandelin]]

Attachment: Part 5: Fix bug 230216

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.

Comment 18

[David Mandelin [:dmandelin]]

Attachment: Part 6: Fix bug 576837

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.

Comment 19

[David Mandelin [:dmandelin]]

Attachment: Part 7: Optional API rename to match JSC

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.

Comment 20

[David Mandelin [:dmandelin]]

Attachment: Part 2 (fixed): Import Yarr rev 86639 unmodified

Comment 21

[David Mandelin [:dmandelin]]

Attachment: Part 3 (fixed): Adapt Yarr rev 86639 to build in Spidermonkey

Comment 22

[Chris Leary [:cdleary] (not checking bugmail)]

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.

Comment 23

[Chris Leary [:cdleary] (not checking bugmail)]

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.

Comment 24

[Chris Leary [:cdleary] (not checking bugmail)]

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.

Comment 25

[Chris Leary [:cdleary] (not checking bugmail)]

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.

Comment 26

[David Mandelin [:dmandelin]]

Landed with a few nit-fixes added.

http://hg.mozilla.org/tracemonkey/rev/de6dfe16fd91

Comment 27

[David Mandelin [:dmandelin]]

Backed out: http://hg.mozilla.org/tracemonkey/rev/6422857800fe

I think I bumped into the bug changing the new ctors. Will push a corrected version soon.

Comment 28

[David Mandelin [:dmandelin]]

Relanded:

http://hg.mozilla.org/tracemonkey/rev/195f8cf8758f

Comment 29

[David Mandelin [:dmandelin]]

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.

Comment 30

[David Mandelin [:dmandelin]]

Attachment: Updated version

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.

Comment 31

[David Mandelin [:dmandelin]]

Looks like it finally stuck:

http://hg.mozilla.org/tracemonkey/rev/cc36a234d0d6
http://hg.mozilla.org/tracemonkey/rev/c8695a65e1e7

Comment 32

[David Mandelin [:dmandelin]]

Attachment: Yarr import patch queue

This is a zip of the corrected patch queue I used to do this Yarr import. It has these patches, in this order:

  del-old-yarr.diff
      Deletes the old Yarr files, from the first import, to get a clean tree.

  import-yarr-rev-86639.diff
      Copies in Yarr files from the given WebKit rev, and nothing more.

  adapt-yarr-rev-86639.diff
      Adapts Yarr and SpiderMonkey to each other, except for memory management
      issues, because those are more likely to change.
  fix-mem.diff
      Adapt memory management.

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

  opt-isset.diff
      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.

Comment 39

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/cc36a234d0d6
http://hg.mozilla.org/mozilla-central/rev/c8695a65e1e7

Comment 41

[j.j.]

Where is this fixed? Please set status flags and Target Milestone