564953 - (PortYarr) JM: Port YARR!

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Avast, ye! Thar be a reg'lar 'spression engine suitin' our needs! Exceptin' unicode, the jollyboat that go by the name YARR be ready for overhaul.

- Load'd-to-the-gunnels Leary

Comment 1

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

As mentioned in email, we must pass tests for all existing regexp behaviors, including non-standards in the RegExp objects, which will be some necessary development on top of the port.

Comment 2

[Brendan Eich [:brendan]]

Cue "Barrett's Privateers" (it doesn't end well -- mind the main truck heading for your legs :-P).

/be

Comment 3

[Mike Shaver (:shaver emeritus)]

I wonder if we can get away with breaking the $1 nonsense that trapped gal, if we do it on trunk.  I wish we could tell how widespread that is, but I don't see another way for web content to converge. :-(

Claire will be following this with interest, I predict: "Mummy, did they rescue the Mary Ellen Carter from the rock?"

Comment 4

[Brendan Eich [:brendan]]

We can't remove RegExp.$1, etc. -- other engines added it to gain web compat (v8, notably), and it's still used:

http://www.google.com/codesearch?as_q=RegExp\.\%24&btnG=Search+Code&hl=en&as_lang=javascript&as_license_restrict=i&as_license=&as_package=&as_filename=&as_case=

We need a carrot to lead developers away from this. The stick only bounces back and hits us in the face.

/be

Comment 5

[Mike Shaver (:shaver emeritus)]

I thought that it was specific state-management behaviour of $1 and friends that was at issue, not the property existing at all.  V8 aims for JSCore compatibility, and gal averred some difference in $1 use between WebKit and Gecko paths, IIRC.

I agree that we can't remove $1 completely!

Comment 6

[Brendan Eich [:brendan]]

I don't know what JSC or V8 do. Maybe we can do likewise. What we do, from memory:

- RegExp.$foo variables are dynamically scoped to the JSContext, so cross-window or cross-frame calls do not affect the callee's RegExp.$foo. I'd be surprised if we can break this. Pleasantly surprised, though!

- We save and restore within lambda replace -- see PreserveRegExpStatics. This is one Andreas ran into.

Andreas, anything else?

/be

Comment 7

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

Attachment: Thar she blows!

WIP patch: untested but compiling interpreted regex execution.

Comment 8

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

Now that I've got the interpreter shim'd alive outside of webkit, plan of execution is to:

1. Make it the back-end for our current JM regexp objects.
2. Make the modifications necessary to pass reftests on interp so to get an idea of what the pain points are (should be helpful going into the the JIT modifications).
3. Enable YARR JIT, hooking it up to the macroasm that lives in JM.
4. Pass reftests for YARR JIT.

Comment 9

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

Is the sticky bit performance-sensitive?

Comment 10

[Brendan Eich [:brendan]]

The /y flag (sticky) is for starting at lastIndex in the target string and not "anchoring" -- not skipping until match. This is critical for performance in any kind of lexical analysis. Lack of it means computing suffix slices of the source string, piling up garbage quadratically (in string chars; linearly in strings).

It should be strictly faster to have y enabled; it should just not anchor. It'll need to use lastIndex though.

/y is supposed to be in the Harmony edition of the standard. Still need Steve Levithan to spec it.

/be

Comment 11

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

RFC: Here's a fun issue with the statics that will probably change existing behavior.

A reftest for our statics shows that changing properties of the RegExp constructor changes the behaviors of subsequently constructed RegExp objects. In beginLine.js (paraphased):

RegExp.multiline = true;
assertEq(String(['123']), String('abc\n123xyz'.match(new RegExp('^\\d+')))));

That's fair enough. What's *not* shown by this reftest, however, is that we currently have *all* regexps query this constructor property during the regular expression bytecode interpretation, so it even affects regular expressions that were previously constructed:

var re = new RegExp('^\\d+');
RegExp.multiline = true;
var match = 'abc\n123xyz'.match(re);
print(match ? "Matched." : "Did not match."); // SM matches.
assertEq(false, re.multiline);

Which is vexing. There are further complications I guess we can talk about if we need more examples.

The only behavior for the RegExp statics that makes intuitive sense to me is that the static values are used on construction (*not* literal syntax) when a string is provided and a flags argument is omitted. It should be ignored in all other cases.

Comment 12

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

Our interpreter seems significantly faster than Yarr's (n.b. JIT disabled) at a first, known-faulty glance. 110ms on regexp-dna for TM top versus around 200ms for Yarr. I had to rip out some WTF memory management classes and replaced their vector implementation with std::vector which makes it an unfair comparison. I'm moving on to hooking up the Yarr JIT instead of fixing those hacks up -- some visible speedup'll calm my nerves.

Comment 13

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

Attachment: SunSpider and V8 comparisons

We still have a few differences in the more complex reftests that need to be hammered out, but it appears that we get about a 10% win on both benchmarks.

V8 benchmark stats:
Compiled:        84.68ms at   96.72%
Uncompiled:      13.25ms at    3.28%

SunSpider benchmark stats:
Compiled:        15.48ms at  100.00%
Uncompiled:       4.17ms at    0.00%

I can augment the JIT compiler to handle the fallback cases if we care enough about those 13ms, but the latest PCRE is BSD licensed, so we can live without complete JIT coverage. ( http://www.pcre.org/licence.txt )

Yo, ho, yo, ho, 'tis a pirate's life for me!

Comment 14

[Brendan Eich [:brendan]]

Got /y working too? Yarr!!

/be

Comment 15

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

(In reply to comment #14)
> Got /y working too? Yarr!!

Yeah, that was a quick hack -- it passes reftests, but hopefully it's sound. When I see compilation with sticky flag I prepend a caret to the proposed source, and during execution I use the last index as the start index. Since the matches are emitted as a buffer of integer two-tuples, you just offset them by the sticky execution index on their way into the RegExpStatics structure. Please do LMK if there's some reason that can't work in the general case.

Comment 16

[Brendan Eich [:brendan]]

Does ^ mean beginning of input/line, or beginning of input as defined by lastIndex?

/be

Comment 17

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

(In reply to comment #16)
> Does ^ mean beginning of input/line, or beginning of input as defined by
> lastIndex?

I mean I munge the regexp source by prepending a caret character and clearing the multiline flag; i.e. similar to a transform |new RegExp(src, flags)| where |flags.indexOf('m') !== -1| turned to |new RegExp('^' + src, flags.replace('m', '')|.

My theory is that this should transform the inital caret into a "beginning of input" assertion and be compatible with any standard (non-hacked) |new RegExp(src, 'ym')| behavior. I'm having trouble coming up with a counterexample.

CC'ing mrbkap.

Comment 18

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

mrbkap: Do we want sticky bit to work this way?

var re = /foo/y; var str = 'blahfoo'; re.lastIndex = 4; re.exec(str).toString() === ["foo"].toString()

(Note that it currently doesn't work if you try to |String.match| instead of |RegExp.exec|...)

Comment 19

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

PCRE and me (sitcom pending) are having some fun times on these last few reftests. The implementation wasn't ready to handle nested groups with quantifiers. I suppose the web must not use those, because they're definitely not to spec in jsc.

Adding support for these requires the engine to be capable of variable sized allocation so it can save/restore nested parts of the match-index vector. The good news is that you don't have to do any additional allocation unless there are nested groups.

Unfortunately, I oriented my quick-fix for nested capture groups and failed to realize it wouldn't work (and needed to) for non-capturing outer groups, so I have a bit more work to do shoving some data in the PCRE bytecode stream. The PCRE compiler is very un-DRY (Write Everything Thrice == WET?), which is why I was trying to avoid this in the first place.

The good news is that all the current reftest failures can be attributed to this and I understand the full scope of the issue. Priority-wise, it sounds like I really need to start jumping on PIC stuff, so hopefully landing this goes smoothly...

Comment 20

[Brendan Eich [:brendan]]

WebKit browsers have less market share, more web compat bugs. Hard to say that "the web" does not handle nested groups w/ quantifiers. In past chats with Olliej he's expressed interest in fixes.

/be

Comment 21

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

Attachment: A few FIXMEs left.

There are still a couple FIXMEs in there and there's one more trick up our sleeves with lazy-copying on |str.replace(lambda)|, but this is the idea. Posting so other people can grab it and test with it. Diff'd against revno 43280. Cube now gets a weird slowdown (10%, about 3ms) I'll look into.

Comment 22

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

Attachment: SunSpider and V8 comparisons

Comment 23

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

Attachment: A few FIXMEs left.

That last posted patch left out a few tiny things. Like the macro assembler. And all of Yarr. This one should be better.

Comment 24

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

Attachment: Macro assembler.

First patch in a series of four:

1. Macro assembler, not enabled in the build. I'll provide a diff against WebKit tip so it's easier to see how we changed things.
2. Yarr/PCRE code, not enabled in the build. Again, I'll provide a diff against WebKit tip.
3. Test tweaks, which demonstrate the few ways in which we're observeably different.
4. Core changes and build enabling. This includes a good amount of API cleanup, since nearly all of jsregexp.cpp as we knew it was removed and largely replaced with the new js::RegExp interface and jsregexpinlines.h. Regexp statics got a cleanup as well to avoid code duplication and encapsulate regexp match data properly -- it conflicted with Andreas' recent cleanup a little bit, but not too badly.

Laundry list:
- Rename hacks.h to something more friendly, maybe the "WTF Bridge to Bona-fide Quality"?
- Remove one or two easy FIXMEs I had left in there.
- One failing XPCShell test, need to investigate.
- A few mochitests still failing, need to investigate. Hopefully not more than a dozen -- I'll know tomorrow.
- Test again running entirely PCRE (no JIT) for better coverage of PCRE code. I haven't done that with mochitests or XPCShell yet, but I did it with the jsreftests.
- Land follow-up patch to make statics save/restore lazy for more win.

Comment 25

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

Forgot to mention that I still also have to take a look at 3D cube, which seemed to regress around 3 milliseconds with this patch last time I checked. That's probably the most pressing thing of all.

Comment 26

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

Attachment: Yarr/PCRE.

Comment 27

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

Attachment: Test tweaks.

Comment 28

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

Attachment: Core changes.

Comment 29

[Andreas Gal :gal]

Do we have a regexp fuzzer?

Comment 30

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

(In reply to comment #29)
> Do we have a regexp fuzzer?

Yeah, Jesse gave me one. Running over the weekend seems like a good idea.

Comment 31

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

Last known bug is fixed -- pushed to try to test other environments, will see how it is in the morning. Updated patches and diffs against webkit will come tomorrow morning if everything is kosher on the other architectures.

Unfortunately, talking to Jesse further, it seems that the current regexp fuzzer implementation doesn't really exercise any of the pieces that are prone to failure in this design.

Comment 32

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

Attachment: Macro assembler.

Comment 33

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

Attachment: Yarr/PCRE.

Comment 34

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

Attachment: Test tweaks.

Comment 35

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

Attachment: Core changes.

Comment 36

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

Attachment: PCRE changes versus WebKit trunk.

Comment 37

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

Attachment: Macro assembler versus webkit trunk.

Comment 38

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

Attachment: Yarr versus WebKit trunk.

Comment 39

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

Attachment: JIT files versus WebKit trunk, sans deleted files.

Comment 40

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

Attachment: Core changes.

Comment 41

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

Attachment: Test tweaks.

Comment 42

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

Attachment: Yarr/PCRE.

Comment 43

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

Attachment: PCRE changes versus WebKit trunk.

Comment 44

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

Attachment: Yarr versus WebKit trunk.

Comment 45

[Robert Sayre]

Comment on attachment 453270 [details] [diff] [review]
PCRE changes versus WebKit trunk.

#include "yarr/jswtfbbq.h"

no. rename.

Comment 46

[Robert Sayre]

Andreas and Luke will split duty on reviewing this, and we'll need a legal bug to look at the rollup when everything is ready.

Comment 47

[Andreas Gal :gal]

Comment on attachment 453257 [details] [diff] [review]
JIT files versus WebKit trunk, sans deleted files.

>-#if OS(SYMBIAN)
>+#if WTF_PLATFORM_SYMBIAN
> #include <e32std.h>
> #endif
> 
>-#if CPU(MIPS) && OS(LINUX)
>+#if WTF_CPU_MIPS && WTF_PLATFORM_LINUX
> #include <sys/cachectl.h>
> #endif
> 
>-#if OS(WINCE)
>+#if WTF_PLATFORM_WINCE
> // From pkfuncs.h (private header file from the Platform Builder)
> #define CACHE_SYNC_ALL 0x07F
> extern "C" __declspec(dllimport) void CacheRangeFlush(LPVOID pAddr, DWORD dwLength, DWORD dwFlags);
> #endif

If you already change stuff you can as well use our defines, no? XP_UNIX and __linux__ or whatever we use. I don't really care. Up to you.

> 
> #define JIT_ALLOCATOR_PAGE_SIZE (ExecutableAllocator::pageSize)
> #define JIT_ALLOCATOR_LARGE_ALLOC_SIZE (ExecutableAllocator::pageSize * 4)
> 
>-#if ENABLE(ASSEMBLER_WX_EXCLUSIVE)
>+#if ENABLE_ASSEMBLER_WX_EXCLUSIVE
> #define PROTECTION_FLAGS_RW (PROT_READ | PROT_WRITE)
> #define PROTECTION_FLAGS_RX (PROT_READ | PROT_EXEC)
> #define INITIAL_PROTECTION_FLAGS PROTECTION_FLAGS_RX
> #else
> #define INITIAL_PROTECTION_FLAGS (PROT_READ | PROT_WRITE | PROT_EXEC)
> #endif
> 
>-    static PassRefPtr<ExecutablePool> create(size_t n)
>+    // It should be impossible for us to roll over, because only small
>+    // pools have multiple holders, and they have one holder per chunk
>+    // of generated code, and they only hold 16KB or so of code.
>+    void addRef() { JS_ATOMIC_INCREMENT(&m_refCount); }
>+    void release() { if (JS_ATOMIC_DECREMENT(&m_refCount) == 0) delete this; }

You could also do per-thread pools?

>-#elif CPU(MIPS)
>+#elif WTF_CPU_MIPS
>     static void cacheFlush(void* code, size_t size)
>     {
>-#if COMPILER(GCC) && (GCC_VERSION >= 40300)
>+#if WTF_COMPILER_GCC && (GCC_VERSION >= 40300)
> #if WTF_MIPS_ISA_REV(2) && (GCC_VERSION < 40403)
>         int lineSize;
>         asm("rdhwr %0, $1" : "=r" (lineSize));
>         //
>         // Modify "start" and "end" to avoid GCC 4.3.0-4.4.2 bug in
>         // mips_expand_synci_loop that may execute synci one more time.
>         // "start" points to the fisrt byte of the cache line.
>         // "end" points to the last byte of the line before the last cache line.
>@@ -215,81 +249,81 @@ public:
> #else
>         intptr_t end = reinterpret_cast<intptr_t>(code) + size;
>         __builtin___clear_cache(reinterpret_cast<char*>(code), reinterpret_cast<char*>(end));
> #endif
> #else
>         _flush_cache(reinterpret_cast<char*>(code), size, BCACHE);
> #endif
>     }
>-#elif CPU(ARM_THUMB2) && OS(IPHONE_OS)
>+#elif WTF_CPU_ARM_THUMB2 && WTF_PLATFORM_IPHONE
>     static void cacheFlush(void* code, size_t size)
>     {
>         sys_dcache_flush(code, size);
>         sys_icache_invalidate(code, size);
>     }
>-#elif CPU(ARM_THUMB2) && OS(LINUX)
>+#elif WTF_CPU_ARM_THUMB2 && WTF_PLATFORM_LINUX
>     static void cacheFlush(void* code, size_t size)
>     {
>         asm volatile (
>             "push    {r7}\n"
>             "mov     r0, %0\n"
>             "mov     r1, %1\n"
>             "movw    r7, #0x2\n"
>             "movt    r7, #0xf\n"
>             "movs    r2, #0x0\n"
>             "svc     0x0\n"
>             "pop     {r7}\n"
>             :
>             : "r" (code), "r" (reinterpret_cast<char*>(code) + size)
>             : "r0", "r1", "r2");
>     }
>-#elif OS(SYMBIAN)
>+#elif WTF_PLATFORM_SYMBIAN
>     static void cacheFlush(void* code, size_t size)
>     {
>         User::IMB_Range(code, static_cast<char*>(code) + size);
>     }
>-#elif CPU(ARM_TRADITIONAL) && OS(LINUX) && COMPILER(RVCT)
>+#elif WTF_CPU_ARM_TRADITIONAL && WTF_PLATFORM_LINUX && WTF_COMPILER_RVCT
>     static __asm void cacheFlush(void* code, size_t size);
>-#elif CPU(ARM_TRADITIONAL) && OS(LINUX) && COMPILER(GCC)
>+#elif WTF_CPU_ARM_TRADITIONAL && WTF_PLATFORM_LINUX && WTF_COMPILER_RVCT
>     static void cacheFlush(void* code, size_t size)
>     {
>         asm volatile (
>             "push    {r7}\n"
>             "mov     r0, %0\n"
>             "mov     r1, %1\n"
>             "mov     r7, #0xf0000\n"
>             "add     r7, r7, #0x2\n"
>             "mov     r2, #0x0\n"
>             "svc     0x0\n"
>             "pop     {r7}\n"
>             :
>             : "r" (code), "r" (reinterpret_cast<char*>(code) + size)
>             : "r0", "r1", "r2");
>     }
>-#elif OS(WINCE)
>+#elif WTF_PLATFORM_WINCE
>     static void cacheFlush(void* code, size_t size)
>     {
>         CacheRangeFlush(code, size, CACHE_SYNC_ALL);
>     }
> #else
>     #error "The cacheFlush support is missing on this platform."
> #endif

We have code for this in nanojit (in the TM-specific part, avmplus.cpp). Any chance we can share that? Up to you, not important.

>-#include <wtf/VMTags.h>

We should look into the VMTag business. I always wondered what exactly that causes (just accounting/profiling aid?).

Comment 48

[Andreas Gal :gal]

Comment on attachment 453263 [details] [diff] [review]
Test tweaks.

Someone who is really good at reading standards should review this. My favorite is waldo. Feel free to bounce it to someone else though.

Comment 49

[Andreas Gal :gal]

Comment on attachment 453262 [details] [diff] [review]
Core changes.

>+# Needed to "configure" it correctly.  Unfortunately these
>+# flags wind up being applied to all code in js/src, not just
>+# the code in js/src/assembler.
>+CXXFLAGS += -DENABLE_ASSEMBLER=1 -DUSE_SYSTEM_MALLOC=1 -DENABLE_JIT=1
>+

Nasty. Get some help from jimb to do this right.

>+    regExpAllocator = new JSC::ExecutableAllocator();
>+    if (!regExpAllocator)
>+        return false;
>+
>     deflatedStringCache = new js::DeflatedStringCache();
>     if (!deflatedStringCache || !deflatedStringCache->init())
>         return false;

We leak if we exit here, don't we? Not sure that matters. Just for the record. XPCOM has an auto_ptr thing. Our C++ overlord should hack one up for us.

>+    void setMultiline(bool enabled) { flags = enabled ? (flags | JSREG_MULTILINE) : (flags & ~JSREG_MULTILINE); }

That line is a little long and hard to read. I don't care much but Brendan will come after you =).

>+    void trace(JSTracer *trc) const {
>+        if (input)
>+            JS_CALL_STRING_TRACER(trc, input, "res->input");
>+    }

mark(). I know we are inconsistent, but trace() is just so poorly named.

>+    bool createInput(jsval &out) const;
>+    bool createLastMatch(jsval &out) const { return makeMatch(0, 0, out); }
>+    bool createLastParen(jsval &out) const;
>+    bool createLeftContext(jsval &out) const;
>+    bool createRightContext(jsval &out) const;

I like passing in jsval *vp, its clearer. Just a nit.

>+class AutoArenaAllocator {
>+    JSArenaPool * const pool;
>+    void        * mark;

Type * name? Really?

>+  public:
>+    explicit AutoArenaAllocator(JSArenaPool *pool) : pool(pool) { mark = JS_ARENA_MARK(pool); }
>+    ~AutoArenaAllocator() { JS_ARENA_RELEASE(pool, mark); }
>+
>+    template <typename T>
>+    T *alloc(size_t elems) {
>+        void *ptr;
>+        JS_ARENA_ALLOCATE(ptr, pool, elems * sizeof(T));
>+        return static_cast<T *>(ptr);
>+    }
>+};
>+
>+class AutoReleasePtr {
>+    JSContext   * const cx;
>+    void        *ptr;

Seriously now.

> class JSAutoResolveFlags
> {

class T {

>+#ifdef CONST
>+#undef CONST
>+#endif
>+

That's kinda random. Why?

>+class AutoObjectLocker {
>+    JSContext   * const cx;
>+    JSObject    * const obj;
>+  public:
>+    AutoObjectLocker(JSContext *cx, JSObject *obj) : cx(cx), obj(obj) { JS_LOCK_OBJ(cx, obj); }
>+    ~AutoObjectLocker() { JS_UNLOCK_OBJ(cx, obj); }
> };

Long line. Move into jsobj.h. Or jslock.h I guess.

>+/* Utility class for creating match array. */

Comment says it all. Move it somewhere.

>+class ArrayAppender
>+{
>+    JSContext   * const cx;
>+    JSObject    * const array;

Aehm ...

(so nice to see all the horrible regexp interp code getting deleted)

>+    bool operator()(jsid id, jsval val) {
>+        return js_DefineProperty(cx, array, id, val, JS_PropertyStub, JS_PropertyStub,
>+                                 JSPROP_ENUMERATE);

I think we write NULL instead of JS_PropertyStub these days.

>+    bool appendIndex(int index) {
>+        return operator()(ATOM_TO_JSID(cx->runtime->atomState.indexAtom), INT_TO_JSVAL(index));

I don't think operator overloading helps clarity here.

>-static JSBool
>+static bool
> SetRegExpLastIndex(JSContext *cx, JSObject *obj, jsdouble lastIndex)
> {
>     JS_ASSERT(obj->isRegExp());
>     return JS_NewNumberValue(cx, lastIndex, obj->addressOfRegExpLastIndex());

Does this show up in any profiles? We shouldn't use JS_ here, there is a faster internal path and its almost never a double anyway. I don't particularly like this part of the code.

>+        if ((origFlags & staticsFlags) != staticsFlags) {
>+            /*
>+             * This regex is lacking flags from the statics, so we must recompile with the new
>+             * flags instead of increffing.

Increffing is not really a word I think.

>-    uintN flags;

Why uintN -> uint32? Its two different things. I saw this before in the patch.

>+    jsdouble lastIndex;

The lastindex-is-double stuff is really nasty. Senseless d-i-d casts all over the place. If you decide to fix, fix in a separate bug.

>+/* Defined in the inlines header to avoid Yarr dependency includes in main header. */

Nasty but ok.

Great work Chris.

Comment 50

[Brendan Eich [:brendan]]

> >+    void addRef() { JS_ATOMIC_INCREMENT(&m_refCount); }
> >+    void release() { if (JS_ATOMIC_DECREMENT(&m_refCount) == 0) delete this; }
> 
> You could also do per-thread pools?

Yeah, let's avoid flushing write buffers, we have JSThread and its data, in which to keep truly ST, lock-free data structures.

> I like passing in jsval *vp, its clearer. Just a nit.

Yeah, style favors out param jsval *vp -- canonical name and make the caller pass the address.

> >+#ifdef CONST
> >+#undef CONST
> >+#endif
> >+
> 
> That's kinda random. Why?

<windows.h> -- hate it.

> >+    bool operator()(jsid id, jsval val) {
> >+        return js_DefineProperty(cx, array, id, val, JS_PropertyStub, JS_PropertyStub,
> >+                                 JSPROP_ENUMERATE);
> 
> I think we write NULL instead of JS_PropertyStub these days.

No, NULL means use class getter or setter -- JS_PropertyStub is the right answer to avoid overhead (traced getter/setter, etc.).

> >+static bool
> > SetRegExpLastIndex(JSContext *cx, JSObject *obj, jsdouble lastIndex)
> > {
> >     JS_ASSERT(obj->isRegExp());
> >     return JS_NewNumberValue(cx, lastIndex, obj->addressOfRegExpLastIndex());
> 
> Does this show up in any profiles? We shouldn't use JS_ here, there is a faster
> internal path and its almost never a double anyway.

JS_NewNumberValue stores an int jsval if it can. But yeah, this should call js_NewNumberInRootedValue.

/be

Comment 51

[Brendan Eich [:brendan]]

Comment the <windows.h> hackaround as I did elsewhere:

$ grep -w CONST *.h
jsparse.h:    // Grr, windows.h or something under it #defines CONST...
jsparse.h:#ifdef CONST
jsparse.h:# undef CONST
jsparse.h:    enum Kind { VAR, CONST, LET, FUNCTION, ARG, UNKNOWN };
jsparse.h:            return CONST;
h

A more Piratical noise than "Grr" is appropriate. Maybe even some cussing.

/be

Comment 52

[Luke Wagner [:luke]]

Comment on attachment 453272 [details] [diff] [review]
Yarr versus WebKit trunk.

There are a few comments on the simple changes that I can make now.  There are a couple areas where the changes aren't so obvious; perhaps you can sit down and walk through them with me.

It looks like RegExpParser.h.orig was included in the patch.

>-    void addSorted(Vector<UChar>& matches, UChar ch)
>+    void addSorted(js::Vector<UChar, 0, js::SystemAllocPolicy>& matches, UChar ch)

It would look a bit nicer to introduce a typedef for each js::Vector instantiation used (it looks like there are ~5) and use those everywhere.

>+            hack::insert(matches, pos, ch);

Per our discussion, I think hack:: Vector functions can be made members of js::Vector.

>-            ASSERT_NOT_REACHED();
>+            JS_NOT_REACHED("blah");

Maybe something a bit more informative?

>     ParenthesesDisjunctionContext* allocParenthesesDisjunctionContext(ByteDisjunction* disjunction, int* output, ByteTerm& term)
>     {
>-        return new(malloc(sizeof(ParenthesesDisjunctionContext) + (((term.atom.parenthesesDisjunction->m_numSubpatterns << 1) - 1) * sizeof(int)) + sizeof(DisjunctionContext) + (disjunction->m_frameSize - 1) * sizeof(uintptr_t))) ParenthesesDisjunctionContext(output, term);
>+      size_t bytes = sizeof(ParenthesesDisjunctionContext) + (((term.atom.parenthesesDisjunction->m_numSubpatterns << 1)) * sizeof(int)) + sizeof(DisjunctionContext) + (disjunction->m_frameSize) * sizeof(uintptr_t);
>+      void *mem = malloc(bytes);
>+        return new(mem) ParenthesesDisjunctionContext(output, term);

It looks like the indentation is off here.

>-        m_parenthesesStack.shrink(stackEnd);
>+        hack::shrink(m_parenthesesStack, stackEnd);

It seems like this can be implemented in terms of resize or shrinkBy.

>     ~BytecodePattern()
>     {
>         deleteAllValues(m_allParenthesesInfo);
>         deleteAllValues(m_userCharacterClasses);
>     }
> 
>-    OwnPtr<ByteDisjunction> m_body;
>+    ByteDisjunction *m_body;

Is this m_body getting deleted somewhere else?

>-            Vector<char> matchesAZaz;
>+            js::Vector<char, 0, js::SystemAllocPolicy> matchesAZaz;

Vector on the stack makes me think that an inline count > 0 would make sense.

>             // If we get here, the alternative matched.
>             if (m_pattern.m_body->m_callFrameSize)
>                 addPtr(Imm32(m_pattern.m_body->m_callFrameSize * sizeof(void*)), stackPointerRegister);
>-
>+            

Patch adds whitespace to empty line.

Comment 53

[Luke Wagner [:luke]]

Comment on attachment 453272 [details] [diff] [review]
Yarr versus WebKit trunk.

It turns out the non-trivial changes were recent changes on WebKit trunk so, with a fresh pull and with the nits picked from comment 52, r+.

Comment 54

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

(In reply to comment #49)
> (From update of attachment 453262 [details] [diff] [review])
> >+# Needed to "configure" it correctly.  Unfortunately these
> >+# flags wind up being applied to all code in js/src, not just
> >+# the code in js/src/assembler.
> >+CXXFLAGS += -DENABLE_ASSEMBLER=1 -DUSE_SYSTEM_MALLOC=1 -DENABLE_JIT=1
> >+
> 
> Nasty. Get some help from jimb to do this right.

Think he's on vacation now -- okay for follow up?

> >+    regExpAllocator = new JSC::ExecutableAllocator();
> >+    if (!regExpAllocator)
> >+        return false;
> >+
> >     deflatedStringCache = new js::DeflatedStringCache();
> >     if (!deflatedStringCache || !deflatedStringCache->init())
> >         return false;
> 
> We leak if we exit here, don't we? Not sure that matters. Just for the record.
> XPCOM has an auto_ptr thing. Our C++ overlord should hack one up for us.

I just followed the style of the rest of the method -- I'll assume it's okay for now unless someone says otherwise.

> I like passing in jsval *vp, its clearer. Just a nit.

I have a weird compulsion to avoid non-null assertions with references. I'll fix as follow up along with double lastIndex.

> >+class AutoArenaAllocator {
> >+    JSArenaPool * const pool;
> >+    void        * mark;
> 
> Type * name? Really?

I also have a weird compulsion to mark member pointers as const when they refer to objects that outlive the current one -- just approximates references. I've gotten rid of these consts in the current patch.

> > class JSAutoResolveFlags
> > {
> 
> class T {

Not according to https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style#Classical_OOP

> >+/* Utility class for creating match array. */
> 
> Comment says it all. Move it somewhere.

I just renamed it RegExpMatchBuilder, since its public interface does match-specific things like setInput. I can generalize it in a follow-up if you think it's generally useful.

> >+    bool operator()(jsid id, jsval val) {
> >+        return js_DefineProperty(cx, array, id, val, JS_PropertyStub, JS_PropertyStub,
> >+                                 JSPROP_ENUMERATE);
> 
> I think we write NULL instead of JS_PropertyStub these days.
> 
> >+    bool appendIndex(int index) {
> >+        return operator()(ATOM_TO_JSID(cx->runtime->atomState.indexAtom), INT_TO_JSVAL(index));
> 
> I don't think operator overloading helps clarity here.

Changed to |builder.append()|.

> >-static JSBool
> >+static bool
> > SetRegExpLastIndex(JSContext *cx, JSObject *obj, jsdouble lastIndex)
> > {
> >     JS_ASSERT(obj->isRegExp());
> >     return JS_NewNumberValue(cx, lastIndex, obj->addressOfRegExpLastIndex());
> 
> Does this show up in any profiles? We shouldn't use JS_ here, there is a faster
> internal path and its almost never a double anyway. I don't particularly like
> this part of the code.

Not sure, will check to js_NewWeaklyRootedNumber inline and check that in the profiles for follow up.

> >+        if ((origFlags & staticsFlags) != staticsFlags) {
> >+            /*
> >+             * This regex is lacking flags from the statics, so we must recompile with the new
> >+             * flags instead of increffing.
> 
> Increffing is not really a word I think.

Will change to "instead of performing increfification," unless you have a recommended alternative. ;-)

> >-    uintN flags;
> 
> Why uintN -> uint32? Its two different things. I saw this before in the patch.

We serialize the flagsword over XDR as a 32b value, so I figured we should fix-width it. I also remember somebody telling me that use of uintN is deprecated?

Comment 55

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

(In reply to comment #50)

Missed this, answers two of my questions in the above. Will get rid of atomics and use NewNumberInRootedValue. Commented the const hack.

Comment 56

[Brendan Eich [:brendan]]

(In reply to comment #54)
> > >+    regExpAllocator = new JSC::ExecutableAllocator();
> > >+    if (!regExpAllocator)
> > >+        return false;
> > >+
> > >     deflatedStringCache = new js::DeflatedStringCache();
> > >     if (!deflatedStringCache || !deflatedStringCache->init())
> > >         return false;
> > 
> > We leak if we exit here, don't we? Not sure that matters. Just for the record.
> > XPCOM has an auto_ptr thing. Our C++ overlord should hack one up for us.
> 
> I just followed the style of the rest of the method -- I'll assume it's okay
> for now unless someone says otherwise.

This is JSRuntime::init, right? It's ok, no leak -- init is fallible and if it fails, its caller JS_NewRuntime will call JS_DestroyRuntime, which null-defends while freeing stuff.

> > >+class AutoArenaAllocator {
> > >+    JSArenaPool * const pool;
> > >+    void        * mark;
> > 
> > Type * name? Really?
> 
> I also have a weird compulsion to mark member pointers as const when they refer
> to objects that outlive the current one -- just approximates references. I've
> gotten rid of these consts in the current patch.


S'ok, I think gal was picking on your space after * style, not the const.

> > >-    uintN flags;
> > 
> > Why uintN -> uint32? Its two different things. I saw this before in the patch.
> 
> We serialize the flagsword over XDR as a 32b value, so I figured we should
> fix-width it. I also remember somebody telling me that use of uintN is
> deprecated?

The uintN typedef could be replaced by unsigned, now that Win16 is dead we use it to mean "native unsigned int, at least 32 bits, and faster if possible than a wrongly sized uint32 or uint64 would be."

You can use uint32 here though, cuz of the XDR dependency. We haven't deprecated uintN and it is all over, but in structs it should be avoided if you have packing or external data requirements that favor a sized int.

/be

Comment 57

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

Attachment: PCRE changes versus WebKit trunk.

Now with less BBQ.

Comment 58

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

Attachment: Yarr js::tl additions.

Comment 59

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

Attachment: PCRE changes versus WebKit trunk.

Bug fix, removed blahs.

Comment 60

[Luke Wagner [:luke]]

Comment on attachment 453845 [details] [diff] [review]
Yarr js::tl additions.

>+template <typename InputIterT, typename ValueT>
>+InputIterT
>+find(InputIterT begin, InputIterT end, const ValueT &target)
>+{
>+    while (begin != end) {
>+        if (*begin++ == target)
>+            return begin;
>+    }
>+    return end;
>+}

I believe find returns a pointer to the wrong element on a match.

>+    bool insert(size_t pos, const T &val);

For consistency, could this take a pointer instead of index?

Comment 61

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

(In reply to comment #60)
> I believe find returns a pointer to the wrong element on a match.

I think that's my cue to go home and sleep some more.

Comment 62

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

Attachment: Yarr js::tl additions.

find was never even used by anything. I seriously need to start coding while conscious.

Comment 63

[Andreas Gal :gal]

Comment on attachment 454649 [details] [diff] [review]
PCRE changes versus WebKit trunk.

We did the review face to face. I gave Chris a handful of nits on paper. Nice work. Lets land it!

Comment 64

[Robert Sayre]

Before this is landed, this bug should contain in-browser SunSpider and V8-v5+ comparisons, with the exact patch that will land vs. TM-tip.

Comment 65

[Andreas Gal :gal]

Also, we might want to land this after fatval.

Comment 66

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

Attachment: Linux x64 browser comparison.

Comment 67

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

Attachment: OSX browser comparison.

Comment 68

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

Attachment: Folded patch.

(In reply to comment #64)
> Before this is landed, this bug should contain in-browser SunSpider and V8-v5+
> comparisons, with the exact patch that will land vs. TM-tip.

Patch to be committed to TraceMonkey with all review notes accounted for.

Comment 69

[David Mandelin [:dmandelin]]

When this lands, please make sure the |assembler| directory is identical to JM's: otherwise I'll be in for merge pain.

Comment 70

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

(In reply to comment #69)
> When this lands, please make sure the |assembler| directory is identical to
> JM's: otherwise I'll be in for merge pain.

We have to figure out how to handle JaegerSpew -- all JaegerSpew is removed in the proposed patch. There was also one reference counting bug I had to fix, will push that into JM.

Comment 71

[Nicholas Nethercote [inactive]]

cdleary, can you rebase the patch so it applies cleanly to the 'moo' repo?  I want to try it but the current patch has bitrotted.

Comment 72

[Robert Sayre]

This bug now blocks beta4.

Comment 73

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

Attachment: Fixes to get PPC building.

A few compile-time checks to switch over to PCRE on PPC.

Comment 74

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

http://hg.mozilla.org/tracemonkey/rev/597254d97174

Comment 75

[Jacob Bramley [:jbramley]]

I think YARR is broken on ARM on JM. I get some weird incorrect results, such as matches failing to match, and I get a weird failure on one trace-test: tests/jaeger/bug555206.js:4: SyntaxError: no input for /a/ 

I've not yet investigated the cause, but this knocks out about 6-7 trace-tests on JM. (I haven't build the TM tree for a while so I don't know if the problem exists there too, though I would expect that it does.)

Comment 76

[Jacob Bramley [:jbramley]]

Oh, YARR is turned off for ARM. ENABLE_YARR_JIT is gated on WTF_CPU_ARM_THUMB2 (amongst other things), but that's Apple's Thumb-2 back-end and isn't used by JM. WTF_CPU_ARM_TRADITIONAL is Szeged's ARM back-end, and is the one to test, though doing so throws up an assertion failure so I haven't changed anything just yet.

Comment 77

[[e-mail]]

Comment on attachment 455789 [details] [diff] [review]
Folded patch.

>+ASFILES +=	TrampolineMasmX64.asm
Some sort of mistake? My Win64 build is busted because of this.

Comment 78

[[e-mail]]

And if I just comment that file out then it still doesn't work, I get random characters creeping into my .replace() results (although there are no matches!)

Comment 79

[Nomis101]

(In reply to comment #77)
> >+ASFILES +=	TrampolineMasmX64.asm
> Some sort of mistake? My Win64 build is busted because of this.
This must be Bug 586887, I think.

Comment 80

[[e-mail]]

(In reply to comment #78)
> I get random characters creeping into my .replace() results
Actually this seems to be a .test() bug. I filed bug 587495.

(In reply to comment #79)
> (In reply to comment #77)
> > >+ASFILES +=	TrampolineMasmX64.asm
> > Some sort of mistake? My Win64 build is busted because of this.
> This must be Bug 586887, I think.
Yes, that covers it, thanks.

Comment 81

[Jacob Bramley [:jbramley]]

(In reply to comment #75)
> I think YARR is broken on ARM on JM.

ARM's implementation of load8 was broken. I fixed this, and also added WTF_CPU_ARM_TRADITIONAL to the ENABLE_YARR_JIT condition. All trace-tests once again pass.

This didn't touch any non-ARM-specific code and I confirmed that x86 tests were not affected, so I just pushed the fix: http://hg.mozilla.org/projects/jaegermonkey/rev/73eb2d14f7ac

Comment 82

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/597254d97174

Comment 83

[Justin Dolske [:Dolske]]

I badly want to mark this bug VARRRRIFIED.

Comment 84

[[e-mail]]

Did you mean to add jswin.h with DOS-style CRLF line endings?