Optimize non-mutation Map and Set operations for String keys

Optimize non-mutation Map and Set operations for String keys

[adamk, 2014-11-26 21:05:09]

adamk@chromium.org changed reviewers:
+ dslomov@chromium.org

[adamk, 2014-11-26 21:05:10]

I think this is ready for review, although I'm sure there's lots more that needs
be cleaned up in the Hydrogen code.

[Dmitry Lomov (no reviews), 2014-11-27 18:18:26]

On a rigth track, first round of comments,

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12112: table,
Add<HConstant>(CollectionType::kNumberOfBucketsIndex),
Why LoadKeyed here? Since the offset is constant, this should be LoadNamedField.
Ditto in other places below

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12142: LoopBuilder loop(this);
Nit: use RAII for Loop/IfBuilders

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12277:
HObjectAccess::ForObservableJSObjectOffset(JSCollection::kTableOffset));
Nit: Add HObjectAccess::ForJSCollectionTable()

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12293: Add<HCallRuntime>(isolate()->factory()->empty_string(),
c_function, 2));
nit: Please pass a name here.
I wonder why everyone just passes an empty string here...
In this case, it is easy: just plumb call->name() from callers.

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12319:
HObjectAccess::ForObservableJSObjectOffset(JSSet::kTableOffset));
Nit: Add HObjectAccess::ForJSCollectionTable()

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12333:
HObjectAccess::ForObservableJSObjectOffset(JSMap::kTableOffset));
Nit: Add HObjectAccess::ForJSCollectionTable()

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12336: static_cast<HValue*>(NULL), FAST_ELEMENTS);
Why LoadKeyed here? Since the offset is constant, this should be LoadNamedField

[adamk, 2014-12-02 21:00:39]

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12112: table,
Add<HConstant>(CollectionType::kNumberOfBucketsIndex),
On 2014/11/27 18:18:26, Dmitry Lomov (chromium) wrote:
> Why LoadKeyed here? Since the offset is constant, this should be
LoadNamedField.
> Ditto in other places below

Done.

In my defense, I was following the code in
HGraphBuilder::BuildUncheckedDictionaryElementLoad(). And now I have to
duplicate FixedArray indexing logic. But I agree it makes sense make this a
named load.

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12142: LoopBuilder loop(this);
On 2014/11/27 18:18:26, Dmitry Lomov (chromium) wrote:
> Nit: use RAII for Loop/IfBuilders

Can you explain what you mean here? Do you mean you want more C++ blocks? Or
something else?

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12277:
HObjectAccess::ForObservableJSObjectOffset(JSCollection::kTableOffset));
On 2014/11/27 18:18:26, Dmitry Lomov (chromium) wrote:
> Nit: Add HObjectAccess::ForJSCollectionTable()

Done.

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12293: Add<HCallRuntime>(isolate()->factory()->empty_string(),
c_function, 2));
On 2014/11/27 18:18:26, Dmitry Lomov (chromium) wrote:
> nit: Please pass a name here.
> I wonder why everyone just passes an empty string here...
> In this case, it is easy: just plumb call->name() from callers.

Done.

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12319:
HObjectAccess::ForObservableJSObjectOffset(JSSet::kTableOffset));
On 2014/11/27 18:18:26, Dmitry Lomov (chromium) wrote:
> Nit: Add HObjectAccess::ForJSCollectionTable()

Done.

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12333:
HObjectAccess::ForObservableJSObjectOffset(JSMap::kTableOffset));
On 2014/11/27 18:18:26, Dmitry Lomov (chromium) wrote:
> Nit: Add HObjectAccess::ForJSCollectionTable()

Done.

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12336: static_cast<HValue*>(NULL), FAST_ELEMENTS);
On 2014/11/27 18:18:26, Dmitry Lomov (chromium) wrote:
> Why LoadKeyed here? Since the offset is constant, this should be
LoadNamedField

Done.

[Dmitry Lomov (no reviews), 2014-12-02 21:26:27]

lgtm % builders

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12142: LoopBuilder loop(this);
On 2014/12/02 21:00:38, adamk wrote:
> On 2014/11/27 18:18:26, Dmitry Lomov (chromium) wrote:
> > Nit: use RAII for Loop/IfBuilders
> 
> Can you explain what you mean here? Do you mean you want more C++ blocks? Or
> something else?

Yes; sorry for being unclear; the pattern is:
{ IfBuilder if(..)
}

etc. You do not need to call End on IfBuilders then

[adamk, 2014-12-02 21:37:58]

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/757143002/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:12142: LoopBuilder loop(this);
On 2014/12/02 21:26:27, Dmitry Lomov (chromium) wrote:
> On 2014/12/02 21:00:38, adamk wrote:
> > On 2014/11/27 18:18:26, Dmitry Lomov (chromium) wrote:
> > > Nit: use RAII for Loop/IfBuilders
> > 
> > Can you explain what you mean here? Do you mean you want more C++ blocks? Or
> > something else?
> 
> Yes; sorry for being unclear; the pattern is:
> { IfBuilder if(..)
> }
> 
> etc. You do not need to call End on IfBuilders then 

That works on IfBuilders, but LoopBuilder just has a DCHECK(finished_) in its
destructor. I've fixed up my ifs, but left the loop as-is. A followup patch
could change LoopBuilder's behavior, but I'd rather not touch it in this one.

[adamk, 2014-12-02 22:37:21]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2014-12-02 22:37:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/757143002/140001

[commit-bot: I haz the power, 2014-12-02 23:06:53]

Committed patchset #8 (id:140001)