Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance: limit buffer size, clear on pipeline exit #24109

Merged
merged 1 commit into from Sep 5, 2019
Merged

Performance: limit buffer size, clear on pipeline exit #24109

merged 1 commit into from Sep 5, 2019

Conversation

@gterzian
Copy link
Member

gterzian commented Aug 29, 2019 •
edited by SimonSapin

Part of #24074

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable
@highfive
Copy link

highfive commented Aug 29, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/window.rs, components/script/dom/performance.rs
  • @KiChjang: components/script/dom/window.rs, components/script/dom/performance.rs
@highfive
Copy link

highfive commented Aug 29, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@gterzian
Copy link
Member Author

gterzian commented Aug 29, 2019

@jdm @asajeffrey The story continues, with a follow-up on #24074

These changes undo the previous changes made at #24072, while keeping the fix in place, and improving compliance with the performance spec.
@gterzian gterzian mentioned this pull request Aug 29, 2019
Open
@gterzian gterzian force-pushed the gterzian:fix_performance_entry_buffer_leak branch from ace5ea2 to 5648268 Aug 29, 2019
@asajeffrey
Copy link
Member

asajeffrey commented Aug 30, 2019

Ooh, nice find! Let me know when you're ready for a review!
@gterzian
Copy link
Member Author

gterzian commented Aug 30, 2019

@asajeffrey r?
@gterzian gterzian requested a review from asajeffrey Sep 3, 2019
@asajeffrey
asajeffrey approved these changes Sep 3, 2019
View changes
Copy link
Member

asajeffrey left a comment

That looks quite a bit nicer, thanks for finding an appropriate bit of the spec!
@asajeffrey
Copy link
Member

asajeffrey commented Sep 3, 2019

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Sep 3, 2019

📌 Commit 5648268 has been approved by asajeffrey
@highfive highfive assigned asajeffrey and unassigned SimonSapin Sep 3, 2019
@bors-servo
Copy link
Contributor

bors-servo commented Sep 3, 2019

⌛ Testing commit 5648268 with merge d45b180...
bors-servo added a commit that referenced this pull request Sep 3, 2019
@bors-servo
Auto merge of #24109 - gterzian:fix_performance_entry_buffer_leak, r=…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
d45b180 
…asajeffrey

Performance: limit buffer size, clear on pipeline exit

<!-- Please describe your changes on the following line: -->

Part of #24074

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/24109)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 3, 2019

💔 Test failed - linux-rel-wpt
@gterzian gterzian force-pushed the gterzian:fix_performance_entry_buffer_leak branch from 5648268 to 9f0f772 Sep 3, 2019
@gterzian
Copy link
Member Author

gterzian commented Sep 3, 2019

@bors-servo try=wpt (increased the size of the buffer to 25 entries, I assume it will fix those tests, and let's see if the other one still passes).
bors-servo added a commit that referenced this pull request Sep 3, 2019
@bors-servo
Auto merge of #24109 - gterzian:fix_performance_entry_buffer_leak, r=…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
0b2dff7 
…<try>

Performance: limit buffer size, clear on pipeline exit

<!-- Please describe your changes on the following line: -->

Part of #24074

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/24109)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 3, 2019

⌛ Trying commit 9f0f772 with merge 0b2dff7...
@gterzian
Copy link
Member Author

gterzian commented Sep 3, 2019

@asajeffrey Actually, just seeing that there is already a concept in place like the one I introduced, so all that is necessary is to add a link to the spec and set the max size to 0 when the window clears its runtime...
@gterzian gterzian force-pushed the gterzian:fix_performance_entry_buffer_leak branch from c6befd3 to e9390e2 Sep 3, 2019
@gterzian gterzian force-pushed the gterzian:fix_performance_entry_buffer_leak branch 2 times, most recently from c2d624e to 892cc83 Sep 3, 2019
@gterzian
performance: clear and disable buffer on pipeline exit
Loading status checks…
6d1136a 
use existing buffer max size
@gterzian gterzian force-pushed the gterzian:fix_performance_entry_buffer_leak branch from 892cc83 to 6d1136a Sep 4, 2019
@gterzian
Copy link
Member Author

gterzian commented Sep 4, 2019

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Sep 4, 2019

⌛ Trying commit 6d1136a with merge 4d13d49...
bors-servo added a commit that referenced this pull request Sep 4, 2019
@bors-servo
Auto merge of #24109 - gterzian:fix_performance_entry_buffer_leak, r=…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
4d13d49 
…<try>

Performance: limit buffer size, clear on pipeline exit

<!-- Please describe your changes on the following line: -->

Part of #24074

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/24109)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 4, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True
@gterzian
Copy link
Member Author

gterzian commented Sep 4, 2019

@asajeffrey r?
@asajeffrey
Copy link
Member

asajeffrey commented Sep 4, 2019

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Sep 4, 2019

📌 Commit 6d1136a has been approved by asajeffrey
@bors-servo
Copy link
Contributor

bors-servo commented Sep 4, 2019

⌛ Testing commit 6d1136a with merge 0aff997...
bors-servo added a commit that referenced this pull request Sep 4, 2019
@bors-servo
Auto merge of #24109 - gterzian:fix_performance_entry_buffer_leak, r=…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
0aff997 
…asajeffrey

Performance: limit buffer size, clear on pipeline exit

<!-- Please describe your changes on the following line: -->

Part of #24074

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/24109)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Sep 5, 2019

@bors-servo retry

  • Network
@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

⌛ Testing commit 6d1136a with merge d430317...
bors-servo added a commit that referenced this pull request Sep 5, 2019
@bors-servo
Auto merge of #24109 - gterzian:fix_performance_entry_buffer_leak, r=…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
d430317 
…asajeffrey

Performance: limit buffer size, clear on pipeline exit

<!-- Please describe your changes on the following line: -->

Part of #24074

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/24109)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: asajeffrey
Pushing d430317 to master...
@bors-servo bors-servo merged commit 6d1136a into servo:master Sep 5, 2019
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@gterzian gterzian deleted the gterzian:fix_performance_entry_buffer_leak branch Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asajeffrey asajeffrey

Assignees

@asajeffrey asajeffrey

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@gterzian @highfive @asajeffrey @bors-servo @jdm @SimonSapin
You can’t perform that action at this time.