Summary: | Simplify hash lookups at RegExp caching | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Renata Hodovan <rhodovan.u-szeged> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, eric, ggaren, oliver, ossy, webkit.review.bot, zherczeg | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Bug Depends on: | 41205, 41240 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Renata Hodovan
2010-06-23 02:46:12 PDT
Created attachment 59496 [details]
Simplify hash lookups in RegExp caching
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? > 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? Created attachment 59657 [details]
Simplify hash lookups in RegExp caching
> ... to call the public RegExp constructor from the public create function.
... to call the *private* RegExp constructor from the public create function.
Comment on attachment 59657 [details]
Simplify hash lookups in RegExp caching
r=me
Nice cleanup.
http://trac.webkit.org/changeset/61833 might have broken GTK Linux 64-bit Debug Reopen because it was rolled out by http://trac.webkit.org/changeset/61845 Comment on attachment 59657 [details] Simplify hash lookups in RegExp caching Clearing flags on attachment: 59657 Committed r61924: <http://trac.webkit.org/changeset/61924> All reviewed patches have been landed. Closing bug. 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. Or you can use "webkit-patch land" which will take care of all that for you. 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. Created attachment 59920 [details]
Patch
Created attachment 59921 [details]
Patch
Comment on attachment 59921 [details]
Patch
r=me
Committed revision 62405. |