Skip to content

script: Reduce GC-rooting in NodeList - #46664

Merged
SimonSapin merged 4 commits into
servo:mainfrom
SimonSapin:nodelist-no-gc
Jul 21, 2026
Merged

script: Reduce GC-rooting in NodeList#46664
SimonSapin merged 4 commits into
servo:mainfrom
SimonSapin:nodelist-no-gc

Conversation

@SimonSapin

Copy link
Copy Markdown
Member

Pass &JsContext in a few more places in order to use *_no_gc APIs. This is a follow-up to #44435

Testing: expecting no change to existing test results

Pass `&JsContext` in a few more places in order to use `*_no_gc` APIs.
This is a follow-up to servo#44435

Signed-off-by: Simon Sapin <simon@igalia.com>
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 20, 2026
},

'NodeList': {
'cx_no_gc': ['Item', 'IndexedGetter'],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is

Suggested change
'cx_no_gc': ['Item', 'IndexedGetter'],
'no_gc': ['Item', 'IndexedGetter'],

not enough?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it is, I just didn’t realize that was a thing

Comment thread components/script/dom/node/nodelist.rs Outdated
pub(crate) fn iter<'a>(
&'a self,
cx: &'a JSContext,
) -> impl Iterator<Item = DomRoot<Node>> + 'a {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a follow-up would be worth to avoid rooting the items, by switching to UnrootedDom?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done. Though I found something worrying along the way: #46665

Signed-off-by: Simon Sapin <simon@igalia.com>
@SimonSapin SimonSapin changed the title script: Avoid GC-rooting the whole child list in NodeRange script: Reduce GC-rooting in NodeList Jul 20, 2026
Signed-off-by: Simon Sapin <simon@igalia.com>

@sagudev sagudev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM.

Comment thread components/script/webdriver_handlers.rs Outdated
fn matching_links(
links: &NodeList,
fn matching_links<'a>(
cx: &'a JSContext,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cx: &'a JSContext,
cx: &'a NoGC,

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Jul 21, 2026
Signed-off-by: Simon Sapin <simon@igalia.com>
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 21, 2026
@SimonSapin
SimonSapin enabled auto-merge July 21, 2026 12:42
@SimonSapin
SimonSapin added this pull request to the merge queue Jul 21, 2026
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 21, 2026
Merged via the queue into servo:main with commit 51b652e Jul 21, 2026
34 checks passed
@SimonSapin
SimonSapin deleted the nodelist-no-gc branch July 21, 2026 13:59
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-awaiting-review There is new code that needs to be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants