885176 - Use Vector's bulk append function instead of appending elements one at a time

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

The Vector class template has append methods which can insert many elements at once. This is simpler than using an explicit loop to insert multiple elements one at a time, and may reduce intermediate allocations in some cases.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: a proposed fix

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 765164 [details] [diff] [review]
a proposed fix

Review of attachment 765164 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks for doing that.

Comment 3

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/adfd8d9bfd0b

Comment 4

[Sean Stangl [:sstangl]]

Comment on attachment 765164 [details] [diff] [review]
a proposed fix

Review of attachment 765164 [details] [diff] [review]:
-----------------------------------------------------------------

Follow-up style nits?

::: js/src/ion/IonAnalysis.cpp
@@ +787,5 @@
>          block->setDomIndex(index);
>  
> +        if (!worklist.append(block->immediatelyDominatedBlocksBegin(),
> +                             block->immediatelyDominatedBlocksEnd()))
> +            return false;

nit: needs {}

@@ +1358,5 @@
>  
>          // Add all immediate dominators to the front of the worklist.
> +        if (!worklist.append(block->immediatelyDominatedBlocksBegin(),
> +                             block->immediatelyDominatedBlocksEnd()))
> +            return false;

{}

::: js/src/ion/ValueNumbering.cpp
@@ +367,5 @@
>  
>          // Add all immediate dominators to the front of the worklist.
> +        if (!worklist.append(block->immediatelyDominatedBlocksBegin(),
> +                             block->immediatelyDominatedBlocksEnd()))
> +            return false;

{}

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/adfd8d9bfd0b

Comment 6

[Dan Gohman [:sunfish]]

Attachment: follow-up style nits

Comment 7

[Sean Stangl [:sstangl]]

Comment on attachment 766697 [details] [diff] [review]
follow-up style nits

Review of attachment 766697 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/IonAnalysis.cpp
@@ +786,5 @@
>          MBasicBlock *block = worklist.popCopy();
>          block->setDomIndex(index);
>  
>          if (!worklist.append(block->immediatelyDominatedBlocksBegin(),
> +                             block->immediatelyDominatedBlocksEnd())) {

style guide requires { on a new line, aligned with the first character of "if".

@@ +1358,5 @@
>          MBasicBlock *block = worklist.popCopy();
>  
>          // Add all immediate dominators to the front of the worklist.
>          if (!worklist.append(block->immediatelyDominatedBlocksBegin(),
> +                             block->immediatelyDominatedBlocksEnd())) {

and here

::: js/src/ion/ValueNumbering.cpp
@@ +366,5 @@
>          IonSpew(IonSpew_GVN, "Looking at block %d", block->id());
>  
>          // Add all immediate dominators to the front of the worklist.
>          if (!worklist.append(block->immediatelyDominatedBlocksBegin(),
> +                             block->immediatelyDominatedBlocksEnd())) {

and here :)

Comment 8

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f45d23330a9

Comment 9

[Sean Stangl [:sstangl]]

hmmmmm

Comment 10

[Dan Gohman [:sunfish]]

Attachment: fix the fix

Oops, I applied the patch without your most recent comments fixed. Here's a patch which fixes that.

Comment 11

[Sean Stangl [:sstangl]]

Comment on attachment 766961 [details] [diff] [review]
fix the fix

Review of attachment 766961 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, no worries.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8f45d23330a9