Skip to content

Start marking functions that can transitively trigger a GC#33144

Merged
jdm merged 3 commits intoservo:mainfrom
jdm:can-gc
Aug 22, 2024
Merged

Start marking functions that can transitively trigger a GC#33144
jdm merged 3 commits intoservo:mainfrom
jdm:can-gc

Conversation

@jdm
Copy link
Copy Markdown
Member

@jdm jdm commented Aug 20, 2024

Per #33140, adding this zero-size argument to functions that can transitively GC makes it easier to recognize where hazards are present. This work can be done incrementally and iteratively as time allows.


@jdm jdm requested a review from gterzian as a code owner August 20, 2024 17:12
@jdm
Copy link
Copy Markdown
Member Author

jdm commented Aug 22, 2024

This PR gets us to a point where I can file followups for less experienced contributors to help push it forward. There are many threads to pull!

Copy link
Copy Markdown
Member

@gterzian gterzian left a comment

Choose a reason for hiding this comment

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

LGTM, with one question.

obj: Box<T>,
global: &U,
proto: Option<HandleObject>,
can_gc: CanGc,
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 there a future need to pass it here, as opposed to instantiate it inside the function and hide it from the caller?

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.

Yes. The purpose is to infect callers so it is clear whether any function can trigger a GC.

@jdm jdm added this pull request to the merge queue Aug 22, 2024
Merged via the queue into servo:main with commit 60ef6bc Aug 22, 2024
@jdm jdm deleted the can-gc branch August 22, 2024 12:49
@last-genius
Copy link
Copy Markdown
Contributor

last-genius commented Sep 5, 2024

I'm trying to see if I can pull some of the threads further, and I'm wondering if there's an easier way to discover and poke at autogenerated code.

In particular I'm wondering how to change this trait, used in domparser.rs:

59:impl DOMParserMethods for DOMParser {
60-    // https://w3c.github.io/DOM-Parsing/#the-domparser-interface
61-    fn ParseFromString(
62-        &self,
63-        s: DOMString,
64-        ty: DOMParserBinding::SupportedType,
65-        can_gc: CanGc,
66-    ) -> Fallible<DomRoot<Document>> {

to accept a fourth can_gc argument.

I've found the autogenerated code in out/Bindings/DOMParserBindings.rs:

276:pub trait DOMParserMethods {
277-    fn ParseFromString(&self, str: DOMString, type_: SupportedType) -> Fallible<DomRoot<Document>>;
278-}

And, through poking at CodegenRust.py:

6362- self.cgRoot = CGWrapper(CGIndenter(CGList(methods, "")),
6363:             pre=f"pub trait {descriptor.interface.identifier.name}Methods {{\n",
6364-             post="}")

and Configuration.py:

222-        else:
223:            self.returnType = "DomRoot<%s>" % typeName
224-            self.argumentType = "&%s" % typeName

made sure that this is the code responsible for the pub trait and DomRoot parts. But I can't seem to find what's responsible for the arguments (they seem to be queried from somewhere else and so are not particularly greppable, and are not specified in lists like in this PR) and if these can be modified without making all traits take can_gc

@jdm
Copy link
Copy Markdown
Member Author

jdm commented Sep 5, 2024

@last-genius Good question! A useful model for adding arguments to the generated methods is the InRealm arguments, which are manually annotated in Bindings.conf. there is code in CodegenRust.py that checks for these annotations and adds the argument if they're present.

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.

3 participants