Implement PersistentValueMap, a map that stores UniquePersistent values.

Implement PersistentValueMap, a map that stores UniquePersistent values.
This is preparatory work to get rid of UnsafePersistent in blink.

Related blink changes are here: https://codereview.chromium.org/180363004/

This patch is largely based on https://codereview.chromium.org/175503003/, with some methods added to support the blink change mentioned above.

BUG=
R=dcarney@chromium.org, svenpanne@chromium.org

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

[dcarney, 2014-03-10 11:35:22]

https://codereview.chromium.org/189463019/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode840
include/v8.h:840: V8_INLINE bool SetReturnValueFrom(const K& key,
just SetReturnValue

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode847
include/v8.h:847: GetIsolate()->SetReference(parent, persistent);
create a private function to take a raw pointer

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode851
include/v8.h:851: UniquePersistent<V> Set(const K& key, Local<V> value) {
this can't work, since the Set should 0 the unique, and if it didn't, you'd be
duplicating the cell and returning something that will necessarily destroy it

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode856
include/v8.h:856: UniquePersistent<V> SetWithConfig(const K& key, Local<V>
value,
same here, plus this is not a very generic api

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode5855
include/v8.h:5855: returnValue.Set(Local<V>::New(isolate_, FromVal(value)));
no new handle, just install it directly in return value

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode5884
include/v8.h:5884: if (data.GetValue().IsEmpty()) {  // Zombied handle.
remove this clause

[vogelheim, 2014-03-10 19:00:46]

PTAL.

https://codereview.chromium.org/189463019/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode840
include/v8.h:840: V8_INLINE bool SetReturnValueFrom(const K& key,
On 2014/03/10 11:35:23, dcarney wrote:
> just SetReturnValue

Done.

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode840
include/v8.h:840: V8_INLINE bool SetReturnValueFrom(const K& key,
On 2014/03/10 11:35:23, dcarney wrote:
> just SetReturnValue

Done.

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode847
include/v8.h:847: GetIsolate()->SetReference(parent, persistent);
On 2014/03/10 11:35:23, dcarney wrote:
> create a private function to take a raw pointer

Done.

That is, I'm reusing a private SetReference(..) overload.

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode851
include/v8.h:851: UniquePersistent<V> Set(const K& key, Local<V> value) {
On 2014/03/10 11:35:23, dcarney wrote:
> this can't work, since the Set should 0 the unique, and if it didn't, you'd be
> duplicating the cell and returning something that will necessarily destroy it

Not done.

(As discussed offline, the SetUnique returns a UniquePersistent of the old
value. That should be ok.)

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode856
include/v8.h:856: UniquePersistent<V> SetWithConfig(const K& key, Local<V>
value,
On 2014/03/10 11:35:23, dcarney wrote:
> same here, plus this is not a very generic api

Kinda done. I've changed this to take a UniquePersistent, so that the caller can
set the class id/independent themselves. THis method then becomes an overload of
Set w/ a UniquePersistent.

Honestly, I'm not entirely sure if this is what you meant; please let me know if
not.

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode5855
include/v8.h:5855: returnValue.Set(Local<V>::New(isolate_, FromVal(value)));
On 2014/03/10 11:35:23, dcarney wrote:
> no new handle, just install it directly in return value

Done.

https://codereview.chromium.org/189463019/diff/1/include/v8.h#newcode5884
include/v8.h:5884: if (data.GetValue().IsEmpty()) {  // Zombied handle.
On 2014/03/10 11:35:23, dcarney wrote:
> remove this clause

Done.

https://codereview.chromium.org/189463019/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode853
include/v8.h:853: UniquePersistent<V> Set(const K& key, UniquePersistent<V>&
value) {
I'd like to ASSERT(!value.IsWeak()), since the code makes the assumption it can
SetWeak(..) on this value. How?

(Most parts of v8 seem to use ASSERT from src/checks.h, but I assume one
shouldn't include stuff from src/... here and thus export that header to all v8
clients?)

[dcarney, 2014-03-11 07:44:46]

looking good.  watch the rebase!

https://codereview.chromium.org/189463019/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode853
include/v8.h:853: UniquePersistent<V> Set(const K& key, UniquePersistent<V>&
value) {
UniquePersistent should always be passed by value. not ref

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode853
include/v8.h:853: UniquePersistent<V> Set(const K& key, UniquePersistent<V>&
value) {
On 2014/03/10 19:00:46, vogelheim wrote:
> I'd like to ASSERT(!value.IsWeak()), since the code makes the assumption it
can
> SetWeak(..) on this value. How?
> 
> (Most parts of v8 seem to use ASSERT from src/checks.h, but I assume one
> shouldn't include stuff from src/... here and thus export that header to all
v8
> clients?) 

it's a bit annoying, but we are setting weak immediately, which means we can do
the assert internally. it may already be there

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode5872
include/v8.h:5872: if (hasValue) {
setting an empty returnvalue means settings the returnvalue's default value

[dcarney, 2014-03-11 07:57:18]

https://codereview.chromium.org/189463019/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode5890
include/v8.h:5890: }
we need to dispose the callback data for weak maps

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode5901
include/v8.h:5901: if (!data.GetValue().IsEmpty()) {
this cannot be empty

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode5902
include/v8.h:5902: Traits::DisposeByKey(impl, key);
we should be able to do all this with one disposeCallback, not 3

[vogelheim, 2014-03-11 14:35:42]

https://codereview.chromium.org/189463019/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode853
include/v8.h:853: UniquePersistent<V> Set(const K& key, UniquePersistent<V>&
value) {
On 2014/03/11 07:44:46, dcarney wrote:
> UniquePersistent should always be passed by value. not ref

Done.

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode853
include/v8.h:853: UniquePersistent<V> Set(const K& key, UniquePersistent<V>&
value) {
On 2014/03/11 07:44:46, dcarney wrote:
> On 2014/03/10 19:00:46, vogelheim wrote:
> > I'd like to ASSERT(!value.IsWeak()), since the code makes the assumption it
> can
> > SetWeak(..) on this value. How?
> > 
> > (Most parts of v8 seem to use ASSERT from src/checks.h, but I assume one
> > shouldn't include stuff from src/... here and thus export that header to all
> v8
> > clients?) 
> 
> it's a bit annoying, but we are setting weak immediately, which means we can
do
> the assert internally. it may already be there

What do you mean with "we can do the assert internally"?

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode5872
include/v8.h:5872: if (hasValue) {
On 2014/03/11 07:44:46, dcarney wrote:
> setting an empty returnvalue means settings the returnvalue's default value

This emulates the current behaviour of DOMWrapperMap::SetReturnValueFrom, which
will leave the returnValue unmodified whenever it returns false. I checked the
callers of that code and couldn't tell whether they (deliberately) rely on this
property or not. Hence I opted for the safe choice and preserved the current
behaviour.

The code path is certainly in use, but I suspect in most cases they receive a
'fresh' v8::ReturnValue that is set to the default coming in, so that it
wouldn't make a difference.

How can I determine whether always setting the returnValue modifies the
behaviour or not?


Reference:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode5890
include/v8.h:5890: }
On 2014/03/11 07:57:18, dcarney wrote:
> we need to dispose the callback data for weak maps

This calls PersistentValueMap::Release(), which disposes of the callback data if
Traits::kIsWeak.

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode5901
include/v8.h:5901: if (!data.GetValue().IsEmpty()) {
On 2014/03/11 07:57:18, dcarney wrote:
> this cannot be empty

Removed.

https://codereview.chromium.org/189463019/diff/20001/include/v8.h#newcode5902
include/v8.h:5902: Traits::DisposeByKey(impl, key);
On 2014/03/11 07:57:18, dcarney wrote:
> we should be able to do all this with one disposeCallback, not 3

DisposeCallbackData is called in different contexts from DisposeBy*, e.g. in
Release, when a map value gets deleted or overwritten before it gets garbage
collected, so I guess we need two?

I originally separated out the other two, since seem to be doing two different
things: One does additional cleanup on the value, while the other does
additional cleanup on the map implementation. Those are easily combinable, so I
did that.

If you want to, I can combine all three into one, where I'd pass in all args in
the general case and null-args in the Release-case and then have the traits
classes do an if(..). Not sure that makes the code easier to read; but I'll be
happy to follow your opinion.

[dcarney, 2014-03-11 15:12:05]

lgtm, with some small comments

https://codereview.chromium.org/189463019/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/189463019/diff/40001/include/v8.h#newcode819
include/v8.h:819: template<class K, class V, class Traits>
need to add some small comments to the functions to explain what they do

https://codereview.chromium.org/189463019/diff/40001/include/v8.h#newcode5901
include/v8.h:5901: Traits::Dispose(Release(value).Pass(), data.GetIsolate(),
impl, key);
all function which take an isolate parameter have it as the first parameter by
convention

https://codereview.chromium.org/189463019/diff/40001/include/v8.h#newcode5913
include/v8.h:5913: SetInternal(GetDefaultValue());
not much point in changing this if there's no if(V8_UNLIKELY) ... in SetInternal

[vogelheim, 2014-03-11 16:11:59]

https://codereview.chromium.org/189463019/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/189463019/diff/40001/include/v8.h#newcode819
include/v8.h:819: template<class K, class V, class Traits>
On 2014/03/11 15:12:05, dcarney wrote:
> need to add some small comments to the functions to explain what they do

Done. Please let me know if they're helpful.

https://codereview.chromium.org/189463019/diff/40001/include/v8.h#newcode5901
include/v8.h:5901: Traits::Dispose(Release(value).Pass(), data.GetIsolate(),
impl, key);
On 2014/03/11 15:12:05, dcarney wrote:
> all function which take an isolate parameter have it as the first parameter by
> convention

Done.

https://codereview.chromium.org/189463019/diff/40001/include/v8.h#newcode5913
include/v8.h:5913: SetInternal(GetDefaultValue());
On 2014/03/11 15:12:05, dcarney wrote:
> not much point in changing this if there's no if(V8_UNLIKELY) ... in
SetInternal

Done.

[Sven Panne, 2014-03-12 08:24:34]

lgtm

[dcarney, 2014-03-12 09:11:33]

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