Bug 128624 – Merge AtomicString, Identifier

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

.

[Gavin Barraclough]

Created attachment 223898 [details]
Rage hacked, builds, fixed ASSERTions & infinite loops, but almost all tests still failing!

[Gavin Barraclough]

Created attachment 226987 [details]
New patch

[Gavin Barraclough]

JS Perf looks fine.

All benchmarks:
   <arithmetic>                                     149.9601+-1.3387          148.4964+-0.8381          might be 1.0099x faster
   <geometric>                                       19.2674+-0.0367     ^     19.1212+-0.0932        ^ definitely 1.0076x faster
   <harmonic>                                         4.2925+-0.0201            4.2662+-0.0586          might be 1.0062x faster

[Gavin Barraclough]

Oh, patch needs Changelogs & some of those inlines could do with their own header.

[WebKit Commit Bot]

Attachment 226987 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Identifier.h:113:  The parameter name "r" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Identifier.h:114:  The parameter name "r" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Created attachment 226997 [details]
New fix

Still needs changelog, etc.

[Andreas Kling]

Freakin' awesome!

[Benjamin Poulain]

(In reply to comment #7)
> Freakin' awesome!

+1

[Darin Adler]

Comment on attachment 226997 [details]
New fix

View in context: https://bugs.webkit.org/attachment.cgi?id=226997&action=review

> Source/JavaScriptCore/runtime/SmallStrings.cpp:62
>          m_reps[i] = StringImpl::createSubstringSharingImpl(baseString, i, 1);
> +        m_reps[i] = AtomicString::add(m_reps[i].get());

Why use m_reps[i] for the intermediate value? Why not do this all on one line.

> Source/JavaScriptCore/runtime/VM.cpp:198
> +    , propertyNames(0)

nullptr?

> Source/WTF/wtf/text/AtomicStringTable.cpp:56
> +    for (auto iter : m_table)

Should name this "string", not "iter".

I prefer "auto*" or "auto&" here to make it clear we don’t have any bad copying going on.

> Source/WTF/wtf/text/AtomicStringTable.cpp:60
>  void AtomicStringTable::destroy(AtomicStringTable* table)

Do we even need this function any more since we have a public destructor now?

[Darin Adler]

Comment on attachment 226997 [details]
New fix

View in context: https://bugs.webkit.org/attachment.cgi?id=226997&action=review

Any of these functions that should take or return references rather than pointers?

> Source/JavaScriptCore/runtime/VM.h:296
> +        IdentifierTable* atomicStringTable() const { return identifierTable; }

Consider returning a reference instead of a pointer? Why do we need this function? Just to rename the identifierTable data member?

> Source/WTF/wtf/WTFThreadData.h:55
> +typedef AtomicStringTable IdentifierTable;

Long term would we just get rid of this typedef?

[Gavin Barraclough]

(In reply to comment #9)
> Why use m_reps[i] for the intermediate value? Why not do this all on one line.
> 
> nullptr?
> 
> I prefer "auto*" or "auto&" here to make it clear we don’t have any bad copying going on.

done, done, done.

> > Source/WTF/wtf/text/AtomicStringTable.cpp:60
> >  void AtomicStringTable::destroy(AtomicStringTable* table)
> 
> Do we even need this function any more since we have a public destructor now?

Short answer, yes. Long answer, no.

We still need this because it's used as a part of the mechanism for the main thread / web thread to decide who gets to delete their shared string table. But it's probably an over engineered solution – we probably just an if statement in WTFThreadData's destructor. Let me revise in a follow-on patch.

(In reply to comment #10)
>
> > Source/JavaScriptCore/runtime/VM.h:296
> > +        IdentifierTable* atomicStringTable() const { return identifierTable; }
> 
> Consider returning a reference instead of a pointer?

done,

> Why do we need this function? Just to rename the identifierTable data member?

We use this method so you can call AtomicString::add(VM&, StringImpl*), or AtomicString::add(ExecState&, StringImpl*), template on the first argument type to avoid a layering violation. Hopefully we may be ablate make this go away in a followup patch, if WTFThreadData access is now fast enough we make be able to always look up the string table from the thread data, rather than reading it off the VM in JSC.

> > Source/WTF/wtf/WTFThreadData.h:55
> > +typedef AtomicStringTable IdentifierTable;
> 
> Long term would we just get rid of this typedef?

Yep, definitely. I'll do a followup purge of WTF for use of the work 'Identifier' (e.g. we should probably also remove isIdentifier, all uses -> isAtomic).

[Gavin Barraclough]

Created attachment 227221 [details]
review fixes, windows build fix, separate out new file.

Still missing ChangeLogs, and 2 JSC tests are failing.

[Gavin Barraclough]

Created attachment 227265 [details]
Tests pass, think the JSC/WTF changes are probably complete.

Should probably take another look at that WebCore change, and the Windows build is probably still broke (related). I've added JSC/WTF Changelogs, but still needs some description.

[Gavin Barraclough]

Created attachment 227312 [details]
Patch, with speculative Windows build fix

[Geoffrey Garen]

Comment on attachment 227312 [details]
Patch, with speculative Windows build fix

View in context: https://bugs.webkit.org/attachment.cgi?id=227312&action=review

r=me if it blends

> Source/JavaScriptCore/ChangeLog:9
> +        WTF::StringImpl currently supports two uniquing mechanism - AtomicString and
> +        Identifer - that is one too many.

So, you're saying we should unique our uniquing?

[Gavin Barraclough]

Transmitting file data ............................
Committed revision 165982.

[Gavin Barraclough]

Build fix in revision 165990.

[Andreas Kling]

w00t!

https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22Bindings%2Fcreate-element%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Bindings%2Fget-attribute%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Bindings%2Fget-element-by-id%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Bindings%2Fget-elements-by-tag-name%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Bindings%2Fid-setter%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Bindings%2Fset-attribute%3ARuns%22%5D%5D&days=30

[Ryosuke Niwa]

*** Bug 38015 has been marked as a duplicate of this bug. ***