Bug 180636 – It should be possible to flag a cell for unconditional finalization

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

The GC shouldn't just unconditionally finalize all things in an IsoSubspace.  It should only do it to the candidates.

[Filip Pizlo]

Created attachment 328938 [details]
it's a start

[Filip Pizlo]

Created attachment 328939 [details]
a little more

[Filip Pizlo]

Actually, I shouldn't apply this new API to InferredType/InferredStructure.  Before I started making changes here, if you had to have an unconditional finalizer, you would allocate a malloc object the moment you got a structure and then you'd add the unconditional finalizer during GC.  In trunk right now, you just allocate an object the moment you get a structure (it's now a GC object instead of malloc).  With this patch, you allocate an object and you add yourself to the IsoCellSet.  That's more work than in trunk.

The nice thing about the UnconditionalFinalizer API (that this new API loses) is that it lets you do the additional bookkeeping during GC.  Most objects die young, so most objects that need the bookkeeping never end up doing it.  That means less bookkeeping overall.

[Filip Pizlo]

Created attachment 328943 [details]
more

[Filip Pizlo]

(In reply to Filip Pizlo from comment #3)
> Actually, I shouldn't apply this new API to InferredType/InferredStructure. 
> Before I started making changes here, if you had to have an unconditional
> finalizer, you would allocate a malloc object the moment you got a structure
> and then you'd add the unconditional finalizer during GC.  In trunk right
> now, you just allocate an object the moment you get a structure (it's now a
> GC object instead of malloc).  With this patch, you allocate an object and
> you add yourself to the IsoCellSet.  That's more work than in trunk.
> 
> The nice thing about the UnconditionalFinalizer API (that this new API
> loses) is that it lets you do the additional bookkeeping during GC.  Most
> objects die young, so most objects that need the bookkeeping never end up
> doing it.  That means less bookkeeping overall.

IsoCellSet fixes this.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #5)
> (In reply to Filip Pizlo from comment #3)
> > Actually, I shouldn't apply this new API to InferredType/InferredStructure. 
> > Before I started making changes here, if you had to have an unconditional
> > finalizer, you would allocate a malloc object the moment you got a structure
> > and then you'd add the unconditional finalizer during GC.  In trunk right
> > now, you just allocate an object the moment you get a structure (it's now a
> > GC object instead of malloc).  With this patch, you allocate an object and
> > you add yourself to the IsoCellSet.  That's more work than in trunk.
> > 
> > The nice thing about the UnconditionalFinalizer API (that this new API
> > loses) is that it lets you do the additional bookkeeping during GC.  Most
> > objects die young, so most objects that need the bookkeeping never end up
> > doing it.  That means less bookkeeping overall.
> 
> IsoCellSet fixes this.

To elaborate: the *new* IsoCellSet fixes this by allowing add() to be called during visitChildren.

[Filip Pizlo]

Created attachment 329014 [details]
more

[Filip Pizlo]

Created attachment 329045 [details]
starting to pass some tests

[Filip Pizlo]

Created attachment 329058 [details]
passing even more tests than before

[Filip Pizlo]

Created attachment 329084 [details]
the patch

[Filip Pizlo]

@Yusuke: I think that this new API is going to be a neat alternative to UnconditionalFinalizer. If you put your objects in a IsoSubspace, you can then also create an IsoCellSet (preferably VM-wide) that tracks all of those objects in that subspace that need unconditional finalization.

Alternatively, you can just have the Heap call unconditional finalizers for all of the objects in the subspace. The way it works, you can do unconditional finalization on all objects in a Subspace or objects in a IsoCellSet.  The finalizeUnconditionally() function you put on JSCell does not need to be virtual or in the MethodTable, because template magic is used to call it. If the function is called via Subspace, then it should return void. If it's called via IsoCellSet, it should return IsoCellSet::ForEachResult.

You can add objects to the set at any time - either as soon as you detect the condition that requires unconditional finalization, or even better, in visitChildren if that condition holds. I think that the latter is a bit better.

The neat thing about IsoCellSet is that it just naturally forgets objects that don't survive GC. It does this by participating in sweeping via callbacks from the GC.

[Filip Pizlo]

Created attachment 329085 [details]
the patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:43:  The parameter name "subspace" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:46:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:48:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:50:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:29:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:108:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:129:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:134:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:139:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:144:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:150:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:233:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 12 in 32 files


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

[Filip Pizlo]

Created attachment 329094 [details]
the patch

Maybe fix some builds

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:42:  The parameter name "subspace" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:45:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:47:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:49:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:29:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:108:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:129:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:134:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:139:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:144:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:150:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:233:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 12 in 33 files


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

[Filip Pizlo]

Created attachment 329095 [details]
the patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:42:  The parameter name "subspace" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:45:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:47:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:49:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:29:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:108:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:129:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:134:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:139:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:144:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:150:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:233:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 12 in 33 files


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

[Saam Barati]

Comment on attachment 329095 [details]
the patch

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

A few fly by comments and questions. I need to review this when I have more energy to look closely.

> Source/JavaScriptCore/heap/IsoCellSet.h:40
> +class IsoCellSet : public BasicRawSentinelNode<IsoCellSet> {

Can you elaborate a bit on which operations can race with each other safely so I can better review this? Maybe it's even worth a comment.

> Source/WTF/wtf/ConcurrentBuffer.h:38
> +// It supports storing data that is not copy-constructable but bit-copyable.

Is there some kind of static assert we can add that will help us with this?

> Source/WTF/wtf/ConcurrentBuffer.h:87
> +        T data[1];

Why 1 and not []?

> Source/WTF/wtf/ConcurrentVector.h:87
> +// ConcurrentVector is like SegmentedVector, but suitable for scenarios where one thread appends
> +// elements and another thread continues to access elements at lower indices. Only one thread can
> +// append at a time, so that activity still needs locking.

I think this needs some more explanation.
On the first sentence:

What if I have two threads T1 and T2 running at the same time, I don't think this would work because of how append works.
T1:
    while (true) { concurrentVector.append(T(....)); }
T2: 
  while (true) { print(concurrentVector().last()); }

Do you mean that users of this need to snapshot the size() in a non-racy manner before any augmentation to the vector happens?

Also, I think in the second sentence, you might want to emphasize that users of this API must do the locking. ConcurrentVector does not do the locking for you. Without reading any of the code, and just the class name, I could see myself expecting ConcurrentVector to do the locking for me.

[Saam Barati]

Comment on attachment 329095 [details]
the patch

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

>> Source/JavaScriptCore/heap/IsoCellSet.h:40
>> +class IsoCellSet : public BasicRawSentinelNode<IsoCellSet> {
> 
> Can you elaborate a bit on which operations can race with each other safely so I can better review this? Maybe it's even worth a comment.

Never mind. I see that this is all handled by a CAS. This should mean if many threads are observing the state of this Set, and many threads are mutating it, the state should be sequentially consistent between all threads

[Filip Pizlo]

(In reply to Saam Barati from comment #18)
> Comment on attachment 329095 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329095&action=review
> 
> A few fly by comments and questions. I need to review this when I have more
> energy to look closely.
> 
> > Source/JavaScriptCore/heap/IsoCellSet.h:40
> > +class IsoCellSet : public BasicRawSentinelNode<IsoCellSet> {
> 
> Can you elaborate a bit on which operations can race with each other safely
> so I can better review this? Maybe it's even worth a comment.

It already has a comment above forEachMarkedCell: "It's illegal to call add() or remove() concurrently to this, but self-remove() will work".  That's the only comment we need because add/remove/contains can run concurrently to one another.

> 
> > Source/WTF/wtf/ConcurrentBuffer.h:38
> > +// It supports storing data that is not copy-constructable but bit-copyable.
> 
> Is there some kind of static assert we can add that will help us with this?
> 
> > Source/WTF/wtf/ConcurrentBuffer.h:87
> > +        T data[1];
> 
> Why 1 and not []?

I dunno.  This is the idiom we've used in WebKit for a long time.  I didn't want to try anything else because:

- I have no idea what the exact memory management, sizing, and alignment semantics are of [].

- I have no idea if all compilers we use support [].  If they do support it, I don't know if they support it in a bug-free fashion.

- I know that [1] works for sure.

- I don't see any benefit to [] over [1].

I want to be conservative and do what I know works, since the alternative is crazy bugtail.

> 
> > Source/WTF/wtf/ConcurrentVector.h:87
> > +// ConcurrentVector is like SegmentedVector, but suitable for scenarios where one thread appends
> > +// elements and another thread continues to access elements at lower indices. Only one thread can
> > +// append at a time, so that activity still needs locking.
> 
> I think this needs some more explanation.
> On the first sentence:
> 
> What if I have two threads T1 and T2 running at the same time, I don't think
> this would work because of how append works.
> T1:
>     while (true) { concurrentVector.append(T(....)); }
> T2: 
>   while (true) { print(concurrentVector().last()); }

T2 is not "accessing at lower indices". It's accessing at the end.  My comment was not meant to imply that this would work.

This code won't work because last() may try to read at an index that doesn't have a segment yet and then it will crash.  I'll add a comment above last() and size() that they cannot run concurrently to append().

> 
> Do you mean that users of this need to snapshot the size() in a non-racy
> manner before any augmentation to the vector happens?

No amount of locking inside size() and append() would change the fact that after you read size(), it could change.  So, I'm not sad that size()/last() cannot run concurrently to append().

> 
> Also, I think in the second sentence, you might want to emphasize that users
> of this API must do the locking. ConcurrentVector does not do the locking
> for you. Without reading any of the code, and just the class name, I could
> see myself expecting ConcurrentVector to do the locking for me.

It's not true that users of this API must do locking. You wouldn't need to do locking of the append only happened on the main thread, for example.

[Filip Pizlo]

(In reply to Saam Barati from comment #19)
> Comment on attachment 329095 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329095&action=review
> 
> >> Source/JavaScriptCore/heap/IsoCellSet.h:40
> >> +class IsoCellSet : public BasicRawSentinelNode<IsoCellSet> {
> > 
> > Can you elaborate a bit on which operations can race with each other safely so I can better review this? Maybe it's even worth a comment.
> 
> Never mind. I see that this is all handled by a CAS. This should mean if
> many threads are observing the state of this Set, and many threads are
> mutating it, the state should be sequentially consistent between all threads

And we rely on the GC's existing synchronization in sweeping and elsewhere.  By the time an object is allocated, we are sure that it either has all the bits it needs or we have a race-free way of allocating them because the ConcurrentVector has already been resized and all we need is to allocate the bitmap.

[Filip Pizlo]

Created attachment 329114 [details]
added comments

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:42:  The parameter name "subspace" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:45:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:47:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:49:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:29:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:114:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:135:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:140:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:148:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:153:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:159:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:242:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 12 in 33 files


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

[Filip Pizlo]

Created attachment 329126 [details]
the patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:42:  The parameter name "subspace" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:45:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:47:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:49:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:29:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:114:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:135:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:140:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:148:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:153:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:159:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:242:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 12 in 33 files


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

[Filip Pizlo]

Created attachment 329129 [details]
the patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:42:  The parameter name "subspace" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:45:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:47:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoCellSet.h:49:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:29:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:123:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:144:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:149:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:157:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:162:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:168:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/ConcurrentVector.h:251:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 12 in 33 files


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

[Saam Barati]

Comment on attachment 329129 [details]
the patch

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

r=me
Patch LGTM. I have a couple comments w.r.t documenting some return types. And I have a question about when sweepToFreeList can run and potential races.

> Source/JavaScriptCore/heap/IsoCellSet.cpp:75
> +void IsoCellSet::sweepToFreeList(MarkedBlock::Handle* block)

Can this run concurrently to add()/remove()? I ask because the various filter() calls don't synchronize with the CAS, and also the block->isEmpty()/stale case seems like it could be racy in the same way that forEachCell can't run concurrently to add()/remove()

> Source/JavaScriptCore/heap/IsoCellSet.h:47
> +    bool add(HeapCell* cell);
> +    
> +    bool remove(HeapCell* cell);

Can you add a comment saying if what these return. For example:
add() -> return true when item was added (which is what I think this code does, but I wasn't 100% sure because of the bitmap code)

> Source/JavaScriptCore/heap/IsoCellSetInlines.h:51
> +    return bits->concurrentTestAndClear(atomIndices.atomNumber);

Don't you want ! here so that it returns true if you removed the item? Honestly I might be misreading the code inside BitMap/Atomics. I really think it'd be helpful to have a comment in concurrentTestAnd* saying what the return values mean.

> Source/JavaScriptCore/heap/IsoCellSetInlines.h:64
> +void IsoCellSet::forEachMarkedCell(const Func& func)

I think it might be worth a comment in the header where you say at what points this is legal to run at (or at what points it's illegal to run at). For example, I see that when we start to do a full collection, we clear these bits inside MarkedAllocator.

> Source/JavaScriptCore/heap/IsoCellSetInlines.h:67
> +    (allocator.m_markingNotEmpty & m_blocksWithBits).forEachSetBit(

Is it worth doing something like:
allocator.m_markingNotEmpty.forEachSetBit([&] (size_t index) { if (!m_blocksWithBits[index]) return; ..... rest of function

that way we don't allocate an intermediate bit vector here?

> Source/JavaScriptCore/runtime/InferredType.h:256
> +    // FIXME: This should be Poisoned.

Please link to a bug #

> Source/WTF/wtf/ConcurrentBuffer.h:65
> +        Array* newArray = createArray(newSize);

This is implicitly casting to unsigned which scares me on 64-bit platforms. I think we should crash if we're shaving off bits here.

> Source/WTF/wtf/ConcurrentBuffer.h:82
> +        growExact(std::max(newSize, size * 2));

Maybe worth overflow check on size*2 (at least for 32-bit)?

> Source/WTF/wtf/ConcurrentBuffer.h:98
> +        Array* result = static_cast<Array*>(fastMalloc(OBJECT_OFFSETOF(Array, data) + sizeof(T) * size));

Maybe it's worth an overflow check here (at least for 32-bit this might make sense)

[Saam Barati]

Comment on attachment 329129 [details]
the patch

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

> Source/JavaScriptCore/heap/IsoCellSet.cpp:73
> +void IsoCellSet::didResizeBits(size_t newSize)
> +{
> +    m_blocksWithBits.resize(newSize);
> +    m_bits.grow(newSize);
> +}
> +
> +void IsoCellSet::didRemoveBlock(size_t blockIndex)
> +{
> +    m_blocksWithBits[blockIndex] = false;
> +    m_bits[blockIndex] = nullptr;
> +}

Can this run concurrently to add()/remove()?

[Filip Pizlo]

(In reply to Saam Barati from comment #29)
> Comment on attachment 329129 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329129&action=review
> 
> > Source/JavaScriptCore/heap/IsoCellSet.cpp:73
> > +void IsoCellSet::didResizeBits(size_t newSize)
> > +{
> > +    m_blocksWithBits.resize(newSize);
> > +    m_bits.grow(newSize);
> > +}
> > +
> > +void IsoCellSet::didRemoveBlock(size_t blockIndex)
> > +{
> > +    m_blocksWithBits[blockIndex] = false;
> > +    m_bits[blockIndex] = nullptr;
> > +}
> 
> Can this run concurrently to add()/remove()?

Yes.  We don't remove a block until we prove that none of the cells inside it are live.  Therefore, if you're doing add()/remove() on a live cell, you're not racing with didRemoveBlock().

[Filip Pizlo]

(In reply to Saam Barati from comment #28)
> Comment on attachment 329129 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329129&action=review
> 
> r=me
> Patch LGTM. I have a couple comments w.r.t documenting some return types.
> And I have a question about when sweepToFreeList can run and potential races.
> 
> > Source/JavaScriptCore/heap/IsoCellSet.cpp:75
> > +void IsoCellSet::sweepToFreeList(MarkedBlock::Handle* block)
> 
> Can this run concurrently to add()/remove()? I ask because the various
> filter() calls don't synchronize with the CAS, and also the
> block->isEmpty()/stale case seems like it could be racy in the same way that
> forEachCell can't run concurrently to add()/remove()

Oh man, this is so interesting.  I had to think through this multiple times to identify which aspect of this is a bug and which isn't.

For example, it seems like it's a bug that this is done without locks:

    if (block->isEmpty() || block->areMarksStale()) {
        m_blocksWithBits[block->index()] = false;
        m_bits[block->index()] = nullptr;
        return;
    }

But for the most part, it's totally safe, because if the block is empty during sweep, then there cannot be any concurrent add()/remove() calls to that block.  The only part that isn't safe is manipulating m_blocksWithBits. We need to grab the bitvector lock to do that.

However, the calls to filter() are buggy. I think that Bitmap needs a concurrentFilter(). It's not hard to do that. For example, we can avoid the atomic op if either of these things is true: the filtering would not have cleared any bits (then we do nothing), or the filtering would have cleared all of the bits (then we non-atomically store 0).  I'll implement that and redo perf tests.

> 
> > Source/JavaScriptCore/heap/IsoCellSet.h:47
> > +    bool add(HeapCell* cell);
> > +    
> > +    bool remove(HeapCell* cell);
> 
> Can you add a comment saying if what these return. For example:
> add() -> return true when item was added (which is what I think this code
> does, but I wasn't 100% sure because of the bitmap code)

Yup, will do.

> 
> > Source/JavaScriptCore/heap/IsoCellSetInlines.h:51
> > +    return bits->concurrentTestAndClear(atomIndices.atomNumber);
> 
> Don't you want ! here so that it returns true if you removed the item?

Bitmap's semantics for this function are that it returns the previous value of the bit.  So if we removed the item it would already return true.

> Honestly I might be misreading the code inside BitMap/Atomics. I really
> think it'd be helpful to have a comment in concurrentTestAnd* saying what
> the return values mean.
> 
> > Source/JavaScriptCore/heap/IsoCellSetInlines.h:64
> > +void IsoCellSet::forEachMarkedCell(const Func& func)
> 
> I think it might be worth a comment in the header where you say at what
> points this is legal to run at (or at what points it's illegal to run at).
> For example, I see that when we start to do a full collection, we clear
> these bits inside MarkedAllocator.

What are "these bits"?  I think that the comment already says it:

    // NOTE: It's illegal to call add() or remove() concurrently to this, but self-remove() will work.
    // But it's better if you return Remove instead.

For the particular IsoCellSet we have right now, we guarantee that because add() is only called during visitChildren, remove() is never called, and forEachMarkedCell() is called during unconditional finalization (which cannot run concurrently to visitChildren()).

If we use IsoCellSet for marking constraints, then we'll have to fix this bug, probably by introducing another version of forEachMarkedCell(), which wouldn't do the bit deleting.

> 
> > Source/JavaScriptCore/heap/IsoCellSetInlines.h:67
> > +    (allocator.m_markingNotEmpty & m_blocksWithBits).forEachSetBit(
> 
> Is it worth doing something like:
> allocator.m_markingNotEmpty.forEachSetBit([&] (size_t index) { if
> (!m_blocksWithBits[index]) return; ..... rest of function
> 
> that way we don't allocate an intermediate bit vector here?

What you're suggesting would be way slower.  There is no allocation of bitvectors here.  Check out FastBitVector.h.  `allocator.m_markingNotEmpty & m_blocksWithBits` produces a FastBitVectorOrAdaptor kind of thing, and its forEachSetBit does the & on the fly.

> 
> > Source/JavaScriptCore/runtime/InferredType.h:256
> > +    // FIXME: This should be Poisoned.
> 
> Please link to a bug #

DOne.

> 
> > Source/WTF/wtf/ConcurrentBuffer.h:65
> > +        Array* newArray = createArray(newSize);
> 
> This is implicitly casting to unsigned which scares me on 64-bit platforms.
> I think we should crash if we're shaving off bits here.

I'll just change the code to use size_t consistently.

> 
> > Source/WTF/wtf/ConcurrentBuffer.h:82
> > +        growExact(std::max(newSize, size * 2));
> 
> Maybe worth overflow check on size*2 (at least for 32-bit)?

That'll "just work" thanks to the max() call. :-)

> 
> > Source/WTF/wtf/ConcurrentBuffer.h:98
> > +        Array* result = static_cast<Array*>(fastMalloc(OBJECT_OFFSETOF(Array, data) + sizeof(T) * size));
> 
> Maybe it's worth an overflow check here (at least for 32-bit this might make
> sense)

Fixed.

[Saam Barati]

By "these bits" I meant m_markingNotEmpty

[Filip Pizlo]

(In reply to Saam Barati from comment #32)
> By "these bits" I meant m_markingNotEmpty

Right.  m_markingNotEmpty is cleared during stop-the-world points where nothing else is running.  Other than that, we set bits in it with the bitvector lock held.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/225831/webkit

[Radar WebKit Bug Importer]

<rdar://problem/36011586>