Split Put into Put and Remove

Split Put into Put and Remove

No longer treat the hole as a removal. This removes one branch in
Put and cleans up the API.

BUG=None
LOG=Y
R=adamk@chromium.org, mstarzinger@chromium.org

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

[arv (Not doing code reviews), 2014-05-30 18:45:38]

Fix comments

[arv (Not doing code reviews), 2014-05-30 18:47:08]

https://codereview.chromium.org/309663005/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/309663005/diff/20001/src/objects.h#newcode4143
src/objects.h:4143: static Handle<ObjectHashTable>
Remove(Handle<ObjectHashTable> table,
I wasn't sure if I should add a `bool* was_present` version here as well. It is
never used so I decided against it.

[adamk, 2014-05-30 20:13:57]

https://codereview.chromium.org/309663005/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/309663005/diff/20001/src/objects.h#newcode4219
src/objects.h:4219: static Handle<Derived> Remove(Handle<Derived> table,
Handle<Object> key);
I'm not sure you need this anywhere after you update runtime.cc for my comments.

https://codereview.chromium.org/309663005/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/309663005/diff/20001/src/runtime.cc#newcode1646
src/runtime.cc:1646: Handle<OrderedHashMap> new_table =
OrderedHashMap::Remove(table, key);
You probably want to use the was_present version here, which will allow you to
remove the lookup call on the line above.

https://codereview.chromium.org/309663005/diff/20001/test/cctest/test-ordered...
File test/cctest/test-ordered-hash-table.cc (right):

https://codereview.chromium.org/309663005/diff/20001/test/cctest/test-ordered...
test/cctest/test-ordered-hash-table.cc:179: ordered_map =
OrderedHashMap::Remove(ordered_map, obj);
Probably want to test the was_present version as well...

[arv (Not doing code reviews), 2014-05-30 20:45:58]

Remove move without was_present and use that in the runtime for extra speed

[arv (Not doing code reviews), 2014-05-30 20:53:06]

Ended up requiring was_present in both Remove methods and updated runtime to use
that instead of the extra Lookup.

https://codereview.chromium.org/309663005/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/309663005/diff/20001/src/objects.h#newcode4219
src/objects.h:4219: static Handle<Derived> Remove(Handle<Derived> table,
Handle<Object> key);
On 2014/05/30 20:13:58, adamk wrote:
> I'm not sure you need this anywhere after you update runtime.cc for my
comments.

Done.

https://codereview.chromium.org/309663005/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/309663005/diff/20001/src/runtime.cc#newcode1646
src/runtime.cc:1646: Handle<OrderedHashMap> new_table =
OrderedHashMap::Remove(table, key);
On 2014/05/30 20:13:58, adamk wrote:
> You probably want to use the was_present version here, which will allow you to
> remove the lookup call on the line above.

Done.

https://codereview.chromium.org/309663005/diff/20001/test/cctest/test-ordered...
File test/cctest/test-ordered-hash-table.cc (right):

https://codereview.chromium.org/309663005/diff/20001/test/cctest/test-ordered...
test/cctest/test-ordered-hash-table.cc:179: ordered_map =
OrderedHashMap::Remove(ordered_map, obj);
On 2014/05/30 20:13:58, adamk wrote:
> Probably want to test the was_present version as well...

Done.

[adamk, 2014-05-30 22:34:58]

lgtm, but you'll need an OWNER review of course (adding mstarzinger, cc
rossberg)

[Michael Starzinger, 2014-06-02 13:34:03]

LGTM, just nits.

https://codereview.chromium.org/309663005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/309663005/diff/40001/src/objects.cc#newcode16552
src/objects.cc:16552: Handle<Object> value) {
nit: Can we add an assertion here checking that "value" is not the hole?

https://codereview.chromium.org/309663005/diff/40001/test/cctest/test-diction...
File test/cctest/test-dictionary.cc (right):

https://codereview.chromium.org/309663005/diff/40001/test/cctest/test-diction...
test/cctest/test-dictionary.cc:66: 
nit: Let's add a comment along the lines of ...

// Keys that have been removed are mapped to the hole value.

[arv (Not doing code reviews), 2014-06-02 23:48:50]

Add assert and some comments

[arv (Not doing code reviews), 2014-06-02 23:49:07]

Done

https://codereview.chromium.org/309663005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/309663005/diff/40001/src/objects.cc#newcode16552
src/objects.cc:16552: Handle<Object> value) {
On 2014/06/02 13:34:04, Michael Starzinger wrote:
> nit: Can we add an assertion here checking that "value" is not the hole?

Done.

https://codereview.chromium.org/309663005/diff/40001/test/cctest/test-diction...
File test/cctest/test-dictionary.cc (right):

https://codereview.chromium.org/309663005/diff/40001/test/cctest/test-diction...
test/cctest/test-dictionary.cc:66: 
On 2014/06/02 13:34:04, Michael Starzinger wrote:
> nit: Let's add a comment along the lines of ...
> 
> // Keys that have been removed are mapped to the hole value.

Done.

[adamk, 2014-06-03 00:53:21]

Committed patchset #4 manually as r21616 (presubmit successful).