Bug 41055 – Simplify hash lookups at RegExp caching

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Renata Hodovan]

1) With giving to RegExpCache::create an extra iterator parameter we can simplify some hash lookups.
2) We can collapse the two version of RegExp::create into one, in which we check whether flags are given or not.

[Renata Hodovan]

Created attachment 59496 [details]
Simplify hash lookups in RegExp caching

[Gabor Loki]

Comment on attachment 59496 [details]
Simplify hash lookups in RegExp caching

> +PassRefPtr<RegExp> RegExpCache::create(const UString& patternString, const UString& flags, HashMap<RegExpKey, RefPtr<RegExp> >::iterator iterator) 

The RegExpCacheMap::iterator will look better.

> -PassRefPtr<RegExp> RegExp::create(JSGlobalData* globalData, const UString& pattern)
> -{
> -    return adoptRef(new RegExp(globalData, pattern));
> -}
> -
>  PassRefPtr<RegExp> RegExp::create(JSGlobalData* globalData, const UString& pattern, const UString& flags)
>  {
> +    if (flags.isNull())
> +        return adoptRef(new RegExp(globalData, pattern));
>      return adoptRef(new RegExp(globalData, pattern, flags));
>  }

If you propose this merge, what is the reason not to merge the RegExp constructors and create functions?

[Renata Hodovan]

> The RegExpCacheMap::iterator will look better.
You're right. This way is really better.

> > -PassRefPtr<RegExp> RegExp::create(JSGlobalData* globalData, const UString& pattern)
> > -{
> > -    return adoptRef(new RegExp(globalData, pattern));
> > -}
> > -
> >  PassRefPtr<RegExp> RegExp::create(JSGlobalData* globalData, const UString& pattern, const UString& flags)
> >  {
> > +    if (flags.isNull())
> > +        return adoptRef(new RegExp(globalData, pattern));
> >      return adoptRef(new RegExp(globalData, pattern, flags));
> >  }
> 
> If you propose this merge, what is the reason not to merge the RegExp constructors and create functions?

I agree that we should merge RegExp constructors. But I think it's more prefered to call the public RegExp constructor from the public create function. I have seen examples for this, but maybe I'm wrong. Any comments?

[Renata Hodovan]

Created attachment 59657 [details]
Simplify hash lookups in RegExp caching

[Renata Hodovan]

> ... to call the public RegExp constructor from the public create function. 
... to call the *private* RegExp constructor from the public create function.

[Geoffrey Garen]

Comment on attachment 59657 [details]
Simplify hash lookups in RegExp caching

r=me

Nice cleanup.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61833 might have broken GTK Linux 64-bit Debug

[Csaba Osztrogonác]

Reopen because it was rolled out by http://trac.webkit.org/changeset/61845

[WebKit Commit Bot]

Comment on attachment 59657 [details]
Simplify hash lookups in RegExp caching

Clearing flags on attachment: 59657

Committed r61924: <http://trac.webkit.org/changeset/61924>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Rolled out and reopened again. Sorry, I missed 
to remove cq+ after the previous roll-out. :(

Zoltan, if you would like to land cq+ -ed patch manually, 
next time please set cq- before it, and add comment to
the bug with the revision number.

[Adam Barth]

Or you can use "webkit-patch land" which will take care of all that for you.

[Geoffrey Garen]

The reason this patch was crashing:

In the case where the cache is full, it reuses the insertion iterator after evicting an item. You can't modify the table while maintaining an iterator into the table.

[Geoffrey Garen]

Created attachment 59920 [details]
Patch

[Geoffrey Garen]

Created attachment 59921 [details]
Patch

[Oliver Hunt]

Comment on attachment 59921 [details]
Patch

r=me

[Geoffrey Garen]

Committed revision 62405.