WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED DUPLICATE of
bug 113003
111892
equalIgnoringCase should take a const StringImpl* instead of StringImpl*
https://bugs.webkit.org/show_bug.cgi?id=111892
Summary
equalIgnoringCase should take a const StringImpl* instead of StringImpl*
Eric Seidel (no email)
Reported
2013-03-08 15:24:49 PST
equalIgnoringCase should take a const StringImpl* instead of StringImpl* It's useful sometimes to have a const StringImpl* (a StringImpl you know cannot be ref'd) for thread-safe code. equalIgnoringCase uses StringImpl* for whatever reason. I plan to change it to use const StringImpl* instead.
Attachments
Add attachment
proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1
2013-03-08 22:45:49 PST
> const StringImpl* (a StringImpl you know cannot be ref'd) for thread-safe code
This is quite an indirect way to signal something about thread safety. It seems unlikely that readers of the code would get your intention. In what scenarios do you expect this to be helpful?
Benjamin Poulain
Comment 2
2013-03-08 22:51:38 PST
> This is quite an indirect way to signal something about thread safety. It seems unlikely that readers of the code would get your intention.
You are gonna cry when reading the threaded parser :)
Eric Seidel (no email)
Comment 3
2013-03-08 22:54:24 PST
Certainly threaded programing with RefCounted objects is tricky. :) I'd rather have single-owner strings for the threading case, but for now when I need to grab at Strings that thread doesn't own (like HTMLNames globals), I need to do so with some assurance that they won't get reffed. This is the call-site which motivated this bug:
http://trac.webkit.org/browser/trunk/Source/WebCore/html/parser/BackgroundHTMLParser.cpp#L121
Eric Seidel (no email)
Comment 4
2013-03-08 22:57:55 PST
We considered (and I'm still interested in) not using *Names globals at all, but for now we have these (somewhat hackish and definitely fragile) threadSafeMatch calls for the threaded parser to use.
Eric Seidel (no email)
Comment 5
2013-03-08 23:20:02 PST
I'm not sure I'm going to actually fix this. StringImpl.h is full of non-const StringImpl* arguments. It seems to be the intentional style for that file? I can sympathize with the desire to not spread the const disease. And as ap/benp allude to, the current solution is not really ideal for the threaded parser. We'll probably move to an enum-based system for the threaded parser, autogenerated by the make_names script in parallel to the existing QualifiedName tables. This would be somewhat similar to our ancient enums before the dawn of QualifiedName and AtomicString. :) I want the threaded parser files to be safe to play in w/o hurting yourself, and including HTMLNames (or any string globals) in those files makes them not so. :)
Benjamin Poulain
Comment 6
2013-03-09 00:04:50 PST
I think we just need new objects for the threaded cases. I have a couple of ideas for free conversion of that kind but I never got the time to look into it :( :( I am a fan of TBB-like structure. I wish we had started by adding something like that.
> We considered (and I'm still interested in) not using *Names globals at all, but for now we have these (somewhat hackish and definitely fragile) threadSafeMatch calls for the threaded parser to use.
For the *Names globals, I can make their StringImpl thread safe for the cases you need. This will take quite a bit of work because of something I care about on ARM, which is why I haven't looked into it yet. If that makes your life easier, I can bump it a bit in my list of priorities. (In reply to
comment #5
)
> I'm not sure I'm going to actually fix this. StringImpl.h is full of non-const StringImpl* arguments. It seems to be the intentional style for that file? I can sympathize with the desire to not spread the const disease. And as ap/benp allude to, the current solution is not really ideal for the threaded parser.
I don't see any good reason for the missing const. I assume it is just an oversight. I think it would be good to fix this regardless of the threaded parser issues.
Adam Barth
Comment 7
2013-03-10 08:35:40 PDT
Have we actually measured the performance impact of making StringImpl's ref counting thread-safe? My guess is that it's non-trivial, but we should probably measure instead of guessing. :)
Benjamin Poulain
Comment 8
2013-03-22 15:33:37 PDT
I am fixing the issue as part of a more general improvement:
https://bugs.webkit.org/show_bug.cgi?id=113003
*** This bug has been marked as a duplicate of
bug 113003
***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug