Last Comment Bug 675806 - Static strings should be GC things
: Static strings should be GC things
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Bill McCloskey (:billm)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-01 16:51 PDT by Bill McCloskey (:billm)
Modified: 2011-09-30 01:44 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (67.27 KB, patch)
2011-09-02 17:56 PDT, Bill McCloskey (:billm)
luke: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2011-08-01 16:51:08 PDT
It causes a lot of problems that static atoms are not GC things and don't have mark bits. It's easy to add bugs this way. It's annoying to check for this special case. And Luke says that this is the main obstacle to doing nunbox64.
Comment 1 Andrew McCreight [:mccr8] 2011-08-01 16:57:47 PDT
One example of such a bug is Bug 650519.
Comment 2 Bill McCloskey (:billm) 2011-09-02 17:56:51 PDT
Created attachment 558006 [details] [diff] [review]
patch

I did some benchmarks and this doesn't seem to affect performance. I was hoping for a speedup since we eliminated some branches. Oh well.
Comment 3 Luke Wagner [:luke] 2011-09-06 10:28:00 PDT
Comment on attachment 558006 [details] [diff] [review]
patch

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

Sweet.

::: js/src/jsapi-tests/testIndexToString.cpp
@@ +55,5 @@
>          uint32 u = tests[i].num;
>          JSString *str = js::IndexToString(cx, u);
>          CHECK(str);
>  
> +        if (!js::StaticStrings::hasUintStatic(u))

To avoid redundancy in the context of the whole expression, can you s/hasUintStatic/hasUint/ and similarly for the other StaticStrings members?

::: js/src/jsatom.cpp
@@ +521,5 @@
>  {
>      if (str->isAtom()) {
>          JSAtom &atom = str->asAtom();
>          /* N.B. static atoms are effectively always interned. */
> +        if (ib != InternAtom || js::StaticStrings::isStatic(&atom))

Should be able to drop js::

::: js/src/jsgcmark.cpp
@@ +593,5 @@
>  {
>      if (v.isMarkable()) {
>          JSGCTraceKind kind = v.gcKind();
>          if (kind == JSTRACE_STRING) {
> +            PushMarkStack(gcmarker, (JSString *)v.toGCThing());

Pre-existing, but can this be v.toString()?

::: js/src/vm/String.cpp
@@ +388,5 @@
> +                                   (c) < 36 ? 'a' - 10 : \
> +                                   'A' - 36))
> +
> +#define R FROM_SMALL_CHAR
> +const jschar StaticStrings::fromSmallChar[] = { R6(0) };

can remove

::: js/src/vm/String.h
@@ +675,2 @@
>  
> +struct StaticStrings

can haz class StaticStrings?
Comment 5 Marco Bonardo [::mak] 2011-09-21 02:59:24 PDT
https://hg.mozilla.org/mozilla-central/rev/3c429287dfbe
Comment 6 Mike Hommey [:glandium] 2011-09-30 01:44:48 PDT
Jan, I suspect this broke ia64 (actually, I'm pretty sure it did). Could you check?

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