Make maps in monomorphic IC stubs weak.

Make maps in monomorphic IC stubs weak.

Maps in monomorphic Load, KeyedLoad, Store, KeyedStore, and CompareNil IC
stubs are treated as weak references by the marking visitor.

During generation of an IC stub with a weak map, the stub is appended to the
dependent code array of the map. When the map dies, all stubs in its dependent
code array are invalidated by setting embedded maps to undefined.


BUG=v8:2073
LOG=Y
TEST=cctest/test-heap/WeakMapInMonomorphic*IC
R=mstarzinger@chromium.org, verwaest@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20679

[ulan, 2014-04-02 15:18:28]

PTAL.

[Toon Verwaest, 2014-04-03 15:18:36]

Overall looks good. Just some comments

https://codereview.chromium.org/188783003/diff/120001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/188783003/diff/120001/src/ic.cc#newcode458
src/ic.cc:458: maps.at(i)->AddDependentCode(DependentCode::kWeaklyEmbeddedGroup,
stub);
This means that all maps have 2 pointers to monomorphic ICs, once in the code
cache, and once in the dependency list. Can we unify?

Can we avoid clearing the code cache now that those pointers are weak?

https://codereview.chromium.org/188783003/diff/120001/src/ic.cc#newcode472
src/ic.cc:472: BaseLoadStoreStubCompiler::MissBuiltin(stub->kind()));
4-space indent

https://codereview.chromium.org/188783003/diff/120001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/188783003/diff/120001/src/mark-compact.cc#new...
src/mark-compact.cc:2723: if (code->is_weak_stub()) {
Also IsMarked(code)?

[Toon Verwaest, 2014-04-03 15:39:05]

lgtm

[ulan, 2014-04-03 15:40:25]

https://codereview.chromium.org/188783003/diff/120001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/188783003/diff/120001/src/ic.cc#newcode458
src/ic.cc:458: maps.at(i)->AddDependentCode(DependentCode::kWeaklyEmbeddedGroup,
stub);
On 2014/04/03 15:18:36, Toon Verwaest wrote:
> This means that all maps have 2 pointers to monomorphic ICs, once in the code
> cache, and once in the dependency list. Can we unify?
> 
> Can we avoid clearing the code cache now that those pointers are weak?
As discussed offline, we can probably do that if linear search time is ok.

https://codereview.chromium.org/188783003/diff/120001/src/ic.cc#newcode472
src/ic.cc:472: BaseLoadStoreStubCompiler::MissBuiltin(stub->kind()));
On 2014/04/03 15:18:36, Toon Verwaest wrote:
> 4-space indent

Done.

https://codereview.chromium.org/188783003/diff/120001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/188783003/diff/120001/src/mark-compact.cc#new...
src/mark-compact.cc:2723: if (code->is_weak_stub()) {
On 2014/04/03 15:18:36, Toon Verwaest wrote:
> Also IsMarked(code)?
Good catch, done.

[ulan, 2014-04-07 07:42:19]

Please take another look.

After testing on a few popular websites, I did the following changes:

- instead of maintaining the dependent IC stubs in the dependent code array, I
use the next_code_link field of a stub (which was unused before) to chain the
stubs together in a linked list. The reason for this is that there are maps with
tens of thousands dependent IC stubs which makes the overhead of the dependent
code array too large.

- the head of the list is stored as a separate weak field in the Map. I
considered storing it as an element of the dependent code array, but there are
many Maps that have dependent IC stubs but do not have dependent code array, so
the memory overhead is the same in both cases.

- I added two flags to IC stubs: is_weak_stub and is_invalidated_weak_stub. This
makes sure that we only treat a map weakly after the stub was added to the
dependent IC list of the map.

https://codereview.chromium.org/188783003/diff/180001/src/objects-visiting.cc
File src/objects-visiting.cc (right):

https://codereview.chromium.org/188783003/diff/180001/src/objects-visiting.cc...
src/objects-visiting.cc:215: template <class T>
Code from here to the end of file comes from heap.cc without any change.

[ulan, 2014-04-07 10:26:13]

After discussion with Michael, I moved the head of the dependent IC list into
the dependent code array.

[Michael Starzinger, 2014-04-11 09:01:56]

LGTM.

https://codereview.chromium.org/188783003/diff/200001/src/objects-visiting.h
File src/objects-visiting.h (right):

https://codereview.chromium.org/188783003/diff/200001/src/objects-visiting.h#...
src/objects-visiting.h:490: Object* VisitWeakList(Heap* heap,
suggestion: As discussed offline, I know you only moved this function and didn't
change it, but a few lines of comments describing it here would be nice. :)

https://codereview.chromium.org/188783003/diff/200001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/188783003/diff/200001/src/objects.h#newcode5317
src/objects.h:5317: inline bool can_be_weak_stub() {
nit: This is not really a primitive predicate, grouping it with the other ones
seems counter-intuitive. Can we call it "CanBeWeakStub" instead and separate if
from the predicates by an empty newline?

[ulan, 2014-04-11 09:28:00]

Thank you for review, Michael. I will land after re-running all tests.

My branch was already rebased, so the diff between patch sets includes other
stuff.

Here is how I addressed your comments:

https://codereview.chromium.org/188783003/diff/200001/src/objects-visiting.h
File src/objects-visiting.h (right):

https://codereview.chromium.org/188783003/diff/200001/src/objects-visiting.h#...
src/objects-visiting.h:490: Object* VisitWeakList(Heap* heap,
On 2014/04/11 09:01:56, Michael Starzinger wrote:
> suggestion: As discussed offline, I know you only moved this function and
didn't
> change it, but a few lines of comments describing it here would be nice. :)

Added this comment:

A weak list is single linked list where each element has a weak pointer to the
next element. Given the head of the list, this function removes dead elements
from the list and if requested records slots for next-element pointers. The
template parameter T is a WeakListVisitor that defines how to access the
next-element pointers.

https://codereview.chromium.org/188783003/diff/200001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/188783003/diff/200001/src/objects.h#newcode5317
src/objects.h:5317: inline bool can_be_weak_stub() {
On 2014/04/11 09:01:56, Michael Starzinger wrote:
> nit: This is not really a primitive predicate, grouping it with the other ones
> seems counter-intuitive. Can we call it "CanBeWeakStub" instead and separate
if
> from the predicates by an empty newline?

Done.

[ulan, 2014-04-11 10:36:25]

Committed patchset #6 manually as r20679 (presubmit successful).