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.
> 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?
> 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 :)
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
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.
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. :)
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.
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. :)
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 ***