Skip to content

Conversation

@emilio
Copy link
Member

@emilio emilio commented Mar 9, 2017

This is on top of #15888. Only the second commit needs review.

We put the more recently used item last, so iterating then from left to right is
pointless.


This change is Reviewable

@highfive
Copy link

highfive commented Mar 9, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/cache.rs, components/style/matching.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 9, 2017
@highfive
Copy link

highfive commented Mar 9, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!

@emilio
Copy link
Member Author

emilio commented Mar 9, 2017

r? @SimonSapin or @bholley

@highfive highfive assigned SimonSapin and unassigned Manishearth Mar 9, 2017
@bholley
Copy link
Contributor

bholley commented Mar 9, 2017

File an E-Easy for converting this cache to a VecDeque to avoid memmoving each time we need to pop the least-recently-used entry?

@bors-servo delegate+

@bors-servo
Copy link
Contributor

✌️ @emilio can now approve this pull request

@emilio
Copy link
Member Author

emilio commented Mar 9, 2017

File an E-Easy for converting this cache to a VecDeque to avoid memmoving each time we need to pop the least-recently-used entry?

I don't think that happens a lot of times since we clear it often, but sure.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #15888) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Mar 9, 2017
@emilio emilio force-pushed the lru-back-to-front branch from 5da67e3 to be42f4b Compare March 9, 2017 20:52
@emilio
Copy link
Member Author

emilio commented Mar 9, 2017

@bors-servo r=bholley

@bors-servo
Copy link
Contributor

📌 Commit be42f4b has been approved by bholley

@highfive highfive assigned bholley and unassigned SimonSapin Mar 9, 2017
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Mar 9, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit be42f4b with merge 6280dba...

bors-servo pushed a commit that referenced this pull request Mar 9, 2017
style: Iterate the LRU cache contents from back to front.

This is on top of #15888. Only the second commit needs review.

We put the more recently used item last, so iterating then from left to right is
pointless.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15891)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-css

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Mar 9, 2017
@emilio
Copy link
Member Author

emilio commented Mar 9, 2017

@bors-servo retry

  • Subpixel difference near a border corner, huh

@bors-servo
Copy link
Contributor

⌛ Testing commit be42f4b with merge 0be18f5...

bors-servo pushed a commit that referenced this pull request Mar 9, 2017
style: Iterate the LRU cache contents from back to front.

This is on top of #15888. Only the second commit needs review.

We put the more recently used item last, so iterating then from left to right is
pointless.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15891)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Mar 9, 2017
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Mar 10, 2017
@emilio
Copy link
Member Author

emilio commented Mar 10, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit eecd38a with merge 3b9a074...

bors-servo pushed a commit that referenced this pull request Mar 10, 2017
style: Iterate the LRU cache contents from back to front.

This is on top of #15888. Only the second commit needs review.

We put the more recently used item last, so iterating then from left to right is
pointless.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15891)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
State: approved= try=True

@emilio
Copy link
Member Author

emilio commented Mar 10, 2017

Last commit fixes the reftests failures, I've asked @mbrubeck to take a look.

@mbrubeck
Copy link
Contributor

r=mbrubeck for the layout fix

@emilio
Copy link
Member Author

emilio commented Mar 11, 2017

@bors-servo r=bholley,mbrubeck

@bors-servo
Copy link
Contributor

📌 Commit eecd38a has been approved by bholley,mbrubeck

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 11, 2017
@emilio emilio force-pushed the lru-back-to-front branch from eecd38a to 4bafded Compare March 11, 2017 10:31
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Mar 11, 2017
@emilio
Copy link
Member Author

emilio commented Mar 11, 2017

@bors-servo r=bholley,mbrubeck

@bors-servo
Copy link
Contributor

📌 Commit 4bafded has been approved by bholley,mbrubeck

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 11, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 4bafded with merge 7fa4a94...

bors-servo pushed a commit that referenced this pull request Mar 11, 2017
style: Iterate the LRU cache contents from back to front.

This is on top of #15888. Only the second commit needs review.

We put the more recently used item last, so iterating then from left to right is
pointless.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15891)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: bholley,mbrubeck
Pushing 7fa4a94 to master...

@bors-servo bors-servo merged commit 4bafded into servo:master Mar 11, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 11, 2017
@emilio emilio deleted the lru-back-to-front branch March 23, 2017 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants