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

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 765164 [details] [diff] [review]
a proposed fix
Attachment #765164 - Flags: review?(nicolas.b.pierron)
Comment on attachment 765164 [details] [diff] [review]
a proposed fix

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

Great, thanks for doing that.
Attachment #765164 - Flags: review?(nicolas.b.pierron) → review+
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;

{}
https://hg.mozilla.org/mozilla-central/rev/adfd8d9bfd0b
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Assignee: general → sunfish
(Assignee)

Comment 6

5 years ago
Created attachment 766697 [details] [diff] [review]
follow-up style nits
Attachment #766697 - Flags: review?(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 :)
Attachment #766697 - Flags: review?(sstangl) → review+
hmmmmm
(Assignee)

Comment 10

5 years ago
Created attachment 766961 [details] [diff] [review]
fix the fix

Oops, I applied the patch without your most recent comments fixed. Here's a patch which fixes that.
Attachment #766961 - Flags: review?(sstangl)
Comment on attachment 766961 [details] [diff] [review]
fix the fix

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

Thanks, no worries.
Attachment #766961 - Flags: review?(sstangl) → review+
You need to log in before you can comment on or make changes to this bug.