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

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

We previously used the 'gcreason' namespace but now enum classes are available we can use that instead.

Comment 1

[Ryan Scherich]

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?

Comment 2

[Ryan Scherich]

Attachment: Bug 1512749 - Convert JS::gcreason::Reason to enum class JS:GCReason.

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.

Comment 3

[Ryan Scherich]

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.

Comment 4

[Jon Coppeard (:jonco)]

(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.

Comment 5

[Ryan Scherich]

(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?

Comment 6

[Ryan Scherich]

I removed the unnecessary comments and assertion.

Comment 7

[Jon Coppeard (:jonco)]

(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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13840080fc24
Convert JS::gcreason::Reason to enum class JS:GCReason r=jonco r=mccr8

Comment 9

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/13840080fc24