Comment on attachment 46700[details]
The patch
r-, i think
cachedPrototypeStructure = (Structure*)1
should be
static const Structure* seenFlag = (Structure*)1;
cachedPrototypeStructure = seenFlag
(with a better name for seenFlag)
Created attachment 46718[details]
New implementation for UStringImpl.
Instead, merge the bits into the ref count (ty Alexey for the suggestion!)
No perf degradation, so let's go with this. Restore the hash to a full 32-bits.
Comment on attachment 46704[details]
The patch
This patch is no longer valid, since we're not going to steal bits from the hash.
Will instead probably do something like UStringImpl, stealing bits from the refCount.
Attachment 46718[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/runtime/UStringImpl.h:66: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
JavaScriptCore/runtime/UStringImpl.h:134: More than one command on the same line [whitespace/newline] [4]
JavaScriptCore/runtime/UStringImpl.h:135: More than one command on the same line [whitespace/newline] [4]
Total errors found: 3
Attachment 46756[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/StringImpl.h:105: More than one command on the same line [whitespace/newline] [4]
WebCore/platform/text/StringImpl.h:106: More than one command on the same line [whitespace/newline] [4]
WebCore/platform/text/StringImpl.h:180: More than one command on the same line [whitespace/newline] [4]
Total errors found: 3
Comment on attachment 46756[details]
New WebCore::StringImpl patch
> + OwnPtr<const UChar*> path(fileName.copyWithNullTermination());
This needs to be OwnArrayPtr<const UChar>, not OwnPtr<const UChar*>. Because delete [] and delete without the "[]" are not the same thing.
> + OwnPtr<const UChar*> m_queryWithNullTermination;
Same here.
> + const UChar* copyWithNullTermination() const;
This should probably return PassOwnArrayPtr, not a raw pointer. Otherwise it's too easy to get the type wrong for the smart pointer (as above). Unfortunately, there is no PassOwnArrayPtr as of this writing.
Another good possibility would be to have the function take in an OwnArrayPtr<const UChar>& and set it to point to the newly allocated buffer.
> + static const unsigned s_refCountMask = 0xfffffffe;
I personally prefer capital hex.
review+ assuming you fix the OwnArrayPtr issue.
Comment on attachment 46851[details]
WebCore::StringImpl patch take 3, re-introduce String::charactersWithNullTermination() / StringImpl::hasTerminatingNullCharacter()
Gah! no, this will result in a string's length changing when you access the null terminated form. Nevermind, I'll go with the simple solution & burn another bit. :-(
Attachment 46858[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/StringImpl.h:109: More than one command on the same line [whitespace/newline] [4]
WebCore/platform/text/StringImpl.h:110: More than one command on the same line [whitespace/newline] [4]
WebCore/platform/text/StringImpl.h:184: More than one command on the same line [whitespace/newline] [4]
Total errors found: 3
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 46945[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/runtime/UStringImpl.h:131: More than one command on the same line [whitespace/newline] [4]
JavaScriptCore/runtime/UStringImpl.h:132: More than one command on the same line [whitespace/newline] [4]
Total errors found: 2
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 46945[details]
New UStringImpl patch, without ancillary refactoring changes.
Damn, it removed my review comments!
There weren’t any important ones. One thing I asked was whether we needed more ALWAYS_INLINE.
The webkit patch caused leaks - this is probably also what was causing the JSC patches as originally landed to cause tests crashes on some machines (i.e. they were causing so much leakage DRT actually ran out of memory).
So, reverted in r53575. :-( *sniff*
I'm not sure which (both?) of these patches was reverted. Something tells me this probably doesn't need to be in the http://webkit.org/pending-commit list (by having patches marked r+).
Hey Eric,
All patches are currently reverted, but the first one I think is probably good to go – probably didn't need reverting out in the first place, so I'll leave that r+ed & reland soon.
Removed the r+ from the latter two patches, am working on updated patches for these at the mo.
cheers,
G.
Attachment 48663[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/StringImpl.h:120: More than one command on the same line [whitespace/newline] [4]
WebCore/platform/text/StringImpl.h:121: More than one command on the same line [whitespace/newline] [4]
WebCore/platform/text/StringImpl.h:121: More than one command on the same line in if [whitespace/parens] [4]
WebCore/platform/text/StringImpl.h:195: More than one command on the same line [whitespace/newline] [4]
Total errors found: 4
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 48663[details]
New StringImpl patch, also fix bufferIsInternal bug that was being triggered.
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 54731)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,38 @@
> +2010-02-12 Gavin Barraclough <barraclough@apple.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + https://bugs.webkit.org/show_bug.cgi?id=33731
> + Remove uses of PtrAndFlags from WebCore::StringImpl.
> +
> + These break the OS X Leaks tool. Move the management of null-terminated copies
> + out from StringImpl to String, and use a bits stolen from the refCount to hold the
LIES!
Please fix the change log and add the extra assert in the StringImpl destructor.
Otherwise, r=me.
(In reply to comment #43)
> (From update of attachment 48663[details])
> > Index: WebCore/ChangeLog
> > ===================================================================
> > --- WebCore/ChangeLog (revision 54731)
> > +++ WebCore/ChangeLog (working copy)
> > @@ -1,3 +1,38 @@
> > +2010-02-12 Gavin Barraclough <barraclough@apple.com>
> > +
> > + Reviewed by NOBODY (OOPS!).
> > +
> > + https://bugs.webkit.org/show_bug.cgi?id=33731
> > + Remove uses of PtrAndFlags from WebCore::StringImpl.
> > +
> > + These break the OS X Leaks tool. Move the management of null-terminated copies
> > + out from StringImpl to String, and use a bits stolen from the refCount to hold the
>
> LIES!
>
> Please fix the change log and add the extra assert in the StringImpl
> destructor.
>
> Otherwise, r=me.
Well, I started reviewing it, but Sam was faster than me. Thanks for doing this (and sorry for breaking leaks).
Something to consider:
> + static const unsigned s_refCountMask = 0xFFFFFFF0;
What about
static const unsigned s_refCountMask = UINT_MAX & ~(s_refCountIncrement - 1)
or just
static const unsigned s_refCountMask = ~(s_refCountIncrement - 1)
Landed my previous UStringImpl patch in r54743.
This was r+ from Darin before I marked r-, while debugging an apparent leak. The leak seems to be well understood now, it seems these patches change UStringImpl / StringImpl objects to be 32 bytes instead of 40, and this in turn triggers the pre existing bug (fixed in the StringImpl patch) that if string data falls in the subsequent heap block WebCore::StringImpl would confuse this for an internal buffer & fail to free it.
Attachment 48735[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/runtime/Structure.h:249: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
JavaScriptCore/runtime/Structure.h:264: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
JavaScriptCore/runtime/Structure.h:328: More than one command on the same line [whitespace/newline] [4]
JavaScriptCore/runtime/Structure.h:340: More than one command on the same line [whitespace/newline] [4]
JavaScriptCore/runtime/Structure.h:341: More than one command on the same line [whitespace/newline] [4]
Skipping input 'JavaScriptCore/wtf/PtrAndFlags.h': Can't open for reading
Skipping input 'WebCore/ForwardingHeaders/wtf/PtrAndFlags.h': Can't open for reading
Total errors found: 5
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 48735[details]
Final Patch! Refactor StructureTransitionTable back into Structure, remove PtrAndFlags!
> + , m_usingSingleSlot(true)
The naming scheme for booleans where we make them finish a sentence "structure <xxx>" would mean this should be named m_isUsingSingleSlot.
> + bool structureTransitionTableContains(const StructureTransitionTableHash::Key& key, JSCell* specificValue)
> + Structure* structureTransitionTableGet(const StructureTransitionTableHash::Key& key, JSCell* specificValue) const
> + bool structureTransitionTableHasTransition(const StructureTransitionTableHash::Key& key) const
> + void structureTransitionTableRemove(const StructureTransitionTableHash::Key& key, JSCell* specificValue)
> + void structureTransitionTableAdd(const StructureTransitionTableHash::Key& key, Structure* structure, JSCell* specificValue)
> + TransitionTable* structureTransitionTable() const { ASSERT(!m_usingSingleSlot); return m_transitions.m_table; }
> + void setStructureTransitionTable(TransitionTable* table)
Seems we should name these function members transitionTableXXX since they are members of the Structure class.
It also seems like the header would be a little easier to read if the function bodies were outside the class definition, marked inline. Definitions of functions that are used only inside Structure.cpp can go in that file rather than the header.
r=me
Sending JavaScriptCore/ChangeLog
Sending JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
Sending JavaScriptCore/bytecode/CodeBlock.h
Sending JavaScriptCore/runtime/Structure.cpp
Sending JavaScriptCore/runtime/Structure.h
Sending JavaScriptCore/runtime/StructureTransitionTable.h
Deleting JavaScriptCore/wtf/PtrAndFlags.h
Sending WebCore/ChangeLog
Deleting WebCore/ForwardingHeaders/wtf/PtrAndFlags.h
Transmitting file data .......
Committed revision 54798.
PtrAndFlags is gone, resolved fixed.
A similar problem is know to exist due to our Rope implementation, but tracking that in a new bug ( https://bugs.webkit.org/show_bug.cgi?id=34964 ).
The last patch removed PtrAndFlags.h. However, references to the file still exist:
JavaScriptCore/JavaScriptCore.vcproj/WTF/WTF.vcproj
444: RelativePath="..\..\wtf\PtrAndFlags.h"
JavaScriptGlue/ForwardingHeaders/wtf/PtrAndFlags.h
1:#include <JavaScriptCore/PtrAndFlags.h>
Any cause for worry?
2010-01-15 12:47 PST, Gavin Barraclough
2010-01-15 12:49 PST, Gavin Barraclough
2010-01-15 12:54 PST, Gavin Barraclough
2010-01-15 16:44 PST, Gavin Barraclough
2010-01-16 19:14 PST, Gavin Barraclough
2010-01-18 14:32 PST, Gavin Barraclough
2010-01-18 15:13 PST, Gavin Barraclough
2010-01-19 13:36 PST, Gavin Barraclough
2010-02-12 13:53 PST, Gavin Barraclough
2010-02-12 20:03 PST, Gavin Barraclough
2010-02-14 17:25 PST, Gavin Barraclough