Closed Bug 1512749 Opened 10 months ago Closed 8 months ago

Convert js::gcreason::Reason to an enum class

Categories

(Core :: JavaScript: GC, enhancement, P5)

61 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: jonco, Assigned: ryan.k.scherich, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

We previously used the 'gcreason' namespace but now enum classes are available we can use that instead.
Mentor: jcoppeard
Keywords: good-first-bug
I'm interested in taking a look at this bug. For my understanding, can I clarify a few things about what needs to be done here?

Since the gcreason namespace was mentioned, I'm assuming it's desired to replace gcreason::Reason with an enum class, maybe called "GCReason".
Because the namespace might be/is going away, ExplainReason can't stay there. I assume it's in the namespace to clarify its purpose. Should it be renamed to ExplainGCReason?

Also, I find a few of the comments in gcreason confusing.
One refers to something called JSGC_MAYBEGC, which doesn't seem to exist anywhere. There's other code with a similar prefix, does this comment still make sense in a context I am unaware of or should it just be deleted?
The other comment mentions something like a bucket size of 52 "as of this writing", which doesn't appear to be accurate anymore, unless I am looking in the wrong place. Should this be deleted or updated?
This commit removes the gcreason namespace and replaces the contained Reason
enum with the enum class GCReason. Some casts from GCReason to uint32_t have
been added where implicit conversion was used before. ExplainReason is now
called ExplainGCReason, because it was previously contained in the gcreason
namespace, which is being removed.
I submitted a tentative patch to Phabricator for this patch, and I'm happy to make any other requested changes.

I performed the described renaming and left the comments in GCAPI.h intact as they are.

I also cast from GCReason to uint32_t in a few places, based on already existing casting seen in Runtime.cpp when calling addTelemetry in JSRuntime. Additionally, CycleCollectedJSRuntime::GarbageCollect now accepts a GCReason instead of a uint32_t. In previously existing calls to this function, an implicit conversion was used from Reason to uint32_t, evidently for a comparison in MOZ_ASSERT.

There is some inconsistent naming of parameters throughout the codebase with regards to GCReason; some parameters are named aReason and some are named reason. I left them as they were.
(In reply to Ryan Scherich from comment #3)
Thanks for the patch.  I've reviewed on phabricator and requested review for the DOM parts.

The casting is fine.  Thanks for updating CycleCollectedJSRuntime::GarbageCollect.

> There is some inconsistent naming of parameters throughout the codebase with
> regards to GCReason; some parameters are named aReason and some are named
> reason. I left them as they were.

Outside of /js the convention is to use aParameterNames.
(In reply to Jon Coppeard (:jonco) from comment #4)
> Thanks for the patch.

You're welcome.

> Outside of /js the convention is to use aParameterNames.

Thank you for clarifying this.

Is it appropriate to make the requested changes and resubmit them when review was granted?
I removed the unnecessary comments and assertion.
(In reply to Ryan Scherich from comment #5)

> Is it appropriate to make the requested changes and resubmit them when
> review was granted?

I'm happy for this to land with the requested changes without re-review.
Assignee: nobody → ryan.k.scherich
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13840080fc24
Convert JS::gcreason::Reason to enum class JS:GCReason r=jonco r=mccr8
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.