removal of cx parameter from IsAboutToBeFinalized

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

v3
40.83 KB, patch
gwagner
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
IsAboutToBeFinalized takes cx to access the runtime and get the associated gcCurrentCompartment. Howerever, the runtime can be accessed from the cpmpartment for the thing making the cx parameter unnecessary.
Do you also want to change the JS_IsAboutToBeFinalized function? Whats the procedure to change an API function?
(Assignee)

Comment 2

6 years ago
Created attachment 535871 [details] [diff] [review]
v1

Here is the patch that removes the unneeded cx parameter.
Assignee: general → igor
Attachment #535871 - Flags: review?(anygregor)
(Assignee)

Comment 3

6 years ago
(In reply to comment #1)
> Do you also want to change the JS_IsAboutToBeFinalized function?

Thanks for remainding about it! I will update the patch to always call IsAboutToBeFinalized from xpconnect. After that we would not have JS_IsAboutToBeFinalized calls in the browser and we can wait with any changes to JS_IsAboutToBeFinalized until we have a generational GC or something like that that disrupts the GC logic significantly. 

> Whats the
> procedure to change an API function?

Some discussion on internal and public mail lists is in due.
(Assignee)

Updated

6 years ago
Attachment #535871 - Attachment is obsolete: true
Attachment #535871 - Flags: review?(anygregor)
(Assignee)

Comment 4

5 years ago
Created attachment 595170 [details] [diff] [review]
v2

The patch removes the cx from all internal and public API - per resent discussion on the mailing list this is OK and desirable.
Attachment #595170 - Flags: review?(anygregor)
(Assignee)

Comment 5

5 years ago
Created attachment 595186 [details] [diff] [review]
v3

Here is the right patch!
Attachment #595170 - Attachment is obsolete: true
Attachment #595170 - Flags: review?(anygregor)
Attachment #595186 - Flags: review?(anygregor)
Comment on attachment 595186 [details] [diff] [review]
v3


>-{
>-    JS_ASSERT(thing);
>-    JS_ASSERT(!cx->runtime->gcIncrementalTracer);
>-    return IsAboutToBeFinalized(cx, (gc::Cell *)thing);
>+JS_IsAboutToBeFinalized(void *thing)
>+{
>+    gc::Cell * t = static_cast<gc::Cell *>(thing);

Space around *?

nice!
Attachment #595186 - Flags: review?(anygregor) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6f1d1fbe13a
https://hg.mozilla.org/mozilla-central/rev/d6f1d1fbe13a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.