Last Comment Bug 725595 - Drop native object checks from JS_(Get|Set)ReservedSlot
: Drop native object checks from JS_(Get|Set)ReservedSlot
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 675078 724310
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-09 01:49 PST by Igor Bukanov
Modified: 2012-02-17 05:36 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (17.70 KB, patch)
2012-02-12 12:39 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
v2 (5.14 KB, patch)
2012-02-14 01:37 PST, Igor Bukanov
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2012-02-09 01:49:04 PST
+++ This bug was initially created as a clone of Bug #724310 comment 3 +++

I suppose for historical reasons we allow JS_SetReservedSlot and JS_GetReservedSlot to take a non-native object and do nothing in that case. We should replace those checks with asserts. Also JS_SetReservedSlot calls GCPoke presumably to help the shutdown GC to collect more garbage. I suspect that with cycle collector that is not necessary for quite some time and we can drop that.
Comment 1 Igor Bukanov 2012-02-12 12:29:18 PST
We do not need those native/non-native asserts. The checks in JSObject::(get|set)ReservedSlot should be sufficient to call the API.
Comment 2 Igor Bukanov 2012-02-12 12:39:21 PST
Created attachment 596507 [details] [diff] [review]
v1

The patch makes JS_(Get|Set)ReservedSlot a public API around JSObject::(get|set)ReservedSlot. That allowed to remove inlined SetReservedSlot from jsfriendsapi.h. However I have to keep GetReservedSlot as its use is widespread and its removal may have bad performance implications.
Comment 3 Boris Zbarsky [:bz] 2012-02-13 10:40:49 PST
Actually, we were using js::SetReservedSlot in the new-binding code.  It can get pretty hot during prototype object creation and worse yet object wrapping...

Is there actually a reason to remove it?
Comment 4 Igor Bukanov 2012-02-13 11:56:02 PST
(In reply to Boris Zbarsky (:bz) from comment #3)
> Actually, we were using js::SetReservedSlot in the new-binding code.  It can
> get pretty hot during prototype object creation and worse yet object
> wrapping...
> 
> Is there actually a reason to remove it?

The reason is to minimize exposure of JS internals. But if this a hot issue, then surely we can keep it.
Comment 5 Igor Bukanov 2012-02-14 01:37:56 PST
Created attachment 596944 [details] [diff] [review]
v2

The new patch keeps js::SetReservedSlot and its callers outside js engine as is.
Comment 7 Ed Morley [:emorley] 2012-02-17 05:36:04 PST
https://hg.mozilla.org/mozilla-central/rev/c1b718602a5a

Note You need to log in before you can comment on or make changes to this bug.