If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Improve the GC stack rooting comment

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: njn, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

(Reporter)

Description

5 years ago
As a remote SpiderMonkey hacker in an awkward timezone, I have a very poor understanding of the explicit GC rooting API.  I have some vague rules of thumb that I follow, but I don't know if they are right, if there are pitfalls I'm unaware of, what the consequences of any pitfalls might be, etc, etc.  Any new SpiderMonkey hacker will be in the same boat.

Documentation will help a lot.  There are two pieces of documentation I'm aware of:

- https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Garbage_collection#Explicit_rooting_API.  It's a wiki, and thus out of date, because wikis about code are always out of date.

- The comment at the top of gc/Root.h, which appears to be up to date, because comments about code are usually up to date (certainly more often than wikis are).

I'd love it if the comment in gc/Root.h was about twice its current length.  Suggestions for things it should mention:

- More examples.  They don't need to be long, but more realistic than the existing, partial, two-line example.

- Basic rules of what to do and what not to do.  The wiki has such a list, which is good.  dmandelin told me that rooting inside a loop is bad for performance, that'd be good to mention.

- What are the GC things that need to be wrapped in Rooted<> and Handle<>?  List some or all of them.

- How to deal with GC things that are returned from a function?

- How do you know if a function can trigger a GC?

- More detail about the consequences of getting things wrong.

- Something about the rooting analysis, including how comprehensive it is.  (SkipRoot isn't mentioned, for example.)

It doesn't need to be long, but put yourself in the shoes of a new SpiderMonkey hacker -- it would be greatly helpful to have a single comment that tells you most of what you need to know about the API to use it correctly.
(Reporter)

Comment 1

5 years ago
terrence  njn: yeah.... we're in an in-between state at the moment
          njn: we have good answers to all of your questions, but none of
          them are implemented yet
terrence  njn: we don't want to put documentation non-existent stuff and we
          also don't want to document the wrong way to do things...
njn	  terrence: well, sure. In the meantime, I'm just making shit up and
          hoping that egregiously bogus code is caught in review :/
(In reply to Nicholas Nethercote [:njn] from comment #1)
> terrence  njn: yeah.... we're in an in-between state at the moment
>           njn: we have good answers to all of your questions, but none of
>           them are implemented yet
> terrence  njn: we don't want to put documentation non-existent stuff and we
>           also don't want to document the wrong way to do things...

Fair enough about not publishing documentation for embedders, but that's no excuse for not maintaining a draft document (and also for the benefit of other devs, like Nick). Feel free to put "DRAFT - subject to change and inaccuracy" in large, friendly letters on the cover. On previous projects I've found it a big help to maintain drafts of major design concepts. Or read Nick's last line again:

> It doesn't need to be long, but put yourself in the shoes of a new SpiderMonkey 
> hacker -- it would be greatly helpful to have a single comment that tells you 
> most of what you need to know about the API to use it correctly.
You are right, of course.

Steve has been working on a draft document that may be more up to date: https://wiki.mozilla.org/Sfink/Draft_-_GC_Pointer_Handling.  I've added a link to it from the main GC article as well.
(Reporter)

Comment 4

5 years ago
js/public/RootingAPI.h pretty much has this covered now.
(Reporter)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.