Last Comment Bug 767750 - rm JSScript::evalHashLink
: rm JSScript::evalHashLink
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla16
Assigned To: Luke Wagner [:luke]
:
:
Mentors:
Depends on: 765956 770261
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-23 18:16 PDT by Luke Wagner [:luke]
Modified: 2012-07-12 18:00 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rm (14.62 KB, patch)
2012-07-02 21:18 PDT, Luke Wagner [:luke]
n.nethercote: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-06-23 18:16:48 PDT
With bug 765956, I think we can remove JSScript::evalHashLink by using a Vector in the eval cache instead of embedding the link.  This would save a word and make njn smile.
Comment 1 Nicholas Nethercote [:njn] 2012-06-24 04:15:56 PDT
:)
Comment 2 Luke Wagner [:luke] 2012-07-02 21:18:35 PDT
Created attachment 638585 [details] [diff] [review]
rm

Might as well.  Plus, the eval cache logic was bothering me so I turned it into a proper HashSet.

A few notes:
 - evalCache is cleared on every GC
 - compartment now implies principals (CPG) which is why I replaced the principals subsumption check with compartment equality
 - that EvalKernel assert I removed is already in DirectEval
Comment 3 Nicholas Nethercote [:njn] 2012-07-02 22:17:47 PDT
Comment on attachment 638585 [details] [diff] [review]
rm

Review of attachment 638585 [details] [diff] [review]:
-----------------------------------------------------------------

I know I owe you like 100 reviews but I barely know anything about the EvalCache so this r+ is very rubber-stampy... you should probably get someone else to check it :/

One useful thing I can say:  if you haven't already, please save yourself some trouble later by checking that the sizeof(JSScript) static assertion passes on all eight combinations of:
- 32-bit vs 64-bit
- opt vs debug
- methodjit enabled vs disabled

::: js/src/builtin/Eval.cpp
@@ -63,5 @@
> -
> -    h *= JS_GOLDEN_RATIO;
> -    h >>= 32 - SHIFT;
> -    JS_ASSERT(h < ArrayLength(table_));
> -    return &table_[h];

Good riddance to ad hoc hash tables.
Comment 5 Ed Morley [:emorley] 2012-07-12 05:24:17 PDT
Unfortunately bug 772303 (https://hg.mozilla.org/integration/mozilla-inbound/rev/fad7d06d7dd5) had to be backed out for winxp pgo-only jsreftest failures.

This bug (amongst others) either caused unresolvable (for someone who doesn't work on the JS engine at least) conflicts with the backout of fad7d06d7dd5, or else bugzilla dependencies indicated that one of it's dependants had now been backed out. 

Since there was no one in #developers that could resolve the conflicts, unfortunately this bug has been backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0be4b70b814
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-07-12 18:00:42 PDT
https://hg.mozilla.org/mozilla-central/rev/ebc800948e7a

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