Amend PersistentValueMap.

Amend PersistentValueMap:
- Use the surrounding map (instead of Traits::Impl) for weak callback.
- Provide for a fast reference to a mapped value.
- Restructure Traits to accomondate for the first point above.


[Why?] As discussed, I proceeded to replace Impl with the map.
The problem I encountered with that version is that now the
Traits class depends on itself: The weak-related methods require the
map type in their signature. But the map type includes the Traits class
and hence the Traits class method signatures depend on the specific Traits class. That
makes them practically un-derivable: While you can derive a Traits class
from another one, since the compiler now expects methods with a different
signature. To accommodate, I pulled the dispose traits into the weak traits
class. I also removed the Impl*/MapType* parameter from the Dispose call,
since no implementation seems to need it.

R=dcarney@chromium.org
BUG=

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

[vogelheim, 2014-03-26 19:26:39]

Please have a look. I'll update the dependent CLs shortly.

[dcarney, 2014-03-27 07:45:12]

On 2014/03/26 19:26:39, vogelheim wrote:
> Please have a look. I'll update the dependent CLs shortly.

lgtm, with nits.

I'm sick today, so could you get jochen or marja to land it for you?

also, add some tests for persistentvaluereference

[dcarney, 2014-03-27 07:45:22]

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

https://codereview.chromium.org/212893007/diff/20001/include/v8-util.h#newcod...
include/v8-util.h:256: explicit
PersistentValueReference(PersistentContainerValue value)
expose copy constructor

https://codereview.chromium.org/212893007/diff/20001/include/v8-util.h#newcod...
include/v8-util.h:263: * Helper for V8ValueCache: Get a reference to a map
value.
don't mention v8valuecache

https://codereview.chromium.org/212893007/diff/20001/include/v8-util.h#newcod...
include/v8-util.h:267: * Careful: This is potentially unsafe, in that *any*
change to the
there are limited actions that invalidates these : calling Set or Remove with
the same key, the weak callback for the key, and Clear()

mention them explicitly

https://codereview.chromium.org/212893007/diff/20001/include/v8-util.h#newcod...
include/v8-util.h:283: V8_INLINE PersistentValueReference SetWithReference(
no need for this function since it calls just Set followed by Get

we could instead have a function
UniquePersistent Set(key, value, ValueReference*)
that does all the work of regular Set but sets the valuereference before the
actual call to Set

[dcarney, 2014-03-27 07:55:03]

i realize that everywhere we can kIsWeak, we will have a problem when we
introduce phantom references.  We should introduce an enum somewhere with 2
values, kNotWeak and kWeak and use that everywhere instead of a boolean

[vogelheim, 2014-03-27 14:43:54]

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

https://codereview.chromium.org/212893007/diff/20001/include/v8-util.h#newcod...
include/v8-util.h:256: explicit
PersistentValueReference(PersistentContainerValue value)
On 2014/03/27 07:45:22, dcarney wrote:
> expose copy constructor

Done.

https://codereview.chromium.org/212893007/diff/20001/include/v8-util.h#newcod...
include/v8-util.h:263: * Helper for V8ValueCache: Get a reference to a map
value.
On 2014/03/27 07:45:22, dcarney wrote:
> don't mention v8valuecache

Done.

https://codereview.chromium.org/212893007/diff/20001/include/v8-util.h#newcod...
include/v8-util.h:267: * Careful: This is potentially unsafe, in that *any*
change to the
On 2014/03/27 07:45:22, dcarney wrote:
> there are limited actions that invalidates these : calling Set or Remove with
> the same key, the weak callback for the key, and Clear()
> 
> mention them explicitly

Done.

I'm not entirely convinced of it, though: This is the type of comment that has a
high chance of becoming stale/incorrect, in that changes to the Map may
invalidate the given list of specific functions. Since this comment isn't
located where those changes would happen, the chance that the comment gets
updated is rather slim. Hence my preference for a stern but somewhat unspecific
warning.

https://codereview.chromium.org/212893007/diff/20001/include/v8-util.h#newcod...
include/v8-util.h:283: V8_INLINE PersistentValueReference SetWithReference(
On 2014/03/27 07:45:22, dcarney wrote:
> no need for this function since it calls just Set followed by Get

Removed.

> we could instead have a function
> UniquePersistent Set(key, value, ValueReference*)
> that does all the work of regular Set but sets the valuereference before the
> actual call to Set

Done. Added tests.

[vogelheim, 2014-03-27 14:45:09]

On 2014/03/27 07:55:03, dcarney wrote:
> i realize that everywhere we can kIsWeak, we will have a problem when we
> introduce phantom references.  We should introduce an enum somewhere with 2
> values, kNotWeak and kWeak and use that everywhere instead of a boolean

Done.

If we want to help phantom references, we might add a third value for callbacks
that do not use the mapped value. Not sure if it buys us much, though.

[dcarney, 2014-03-27 16:36:11]

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

https://codereview.chromium.org/212893007/diff/40001/include/v8-util.h#newcode45
include/v8-util.h:45: enum PersistentValueMapCallbackType {
should be persistentcontainercallbacktype, since we'll want to use it for
vectors as well

https://codereview.chromium.org/212893007/diff/40001/include/v8-util.h#newcod...
include/v8-util.h:108: static const PersistentValueMapCallbackType kIsWeak =
kNotWeak;
i think you want to rename this to kCallbackType

[vogelheim, 2014-03-27 17:05:11]

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

https://codereview.chromium.org/212893007/diff/40001/include/v8-util.h#newcode45
include/v8-util.h:45: enum PersistentValueMapCallbackType {
On 2014/03/27 16:36:11, dcarney wrote:
> should be persistentcontainercallbacktype, since we'll want to use it for
> vectors as well

Done.

https://codereview.chromium.org/212893007/diff/40001/include/v8-util.h#newcod...
include/v8-util.h:108: static const PersistentValueMapCallbackType kIsWeak =
kNotWeak;
On 2014/03/27 16:36:11, dcarney wrote:
> i think you want to rename this to kCallbackType

Done.

[marja, 2014-03-28 09:35:56]

Committed patchset #3 manually as r20326 (presubmit successful).