675806 - Static strings should be GC things

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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]]

One example of such a bug is Bug 650519.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 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]]

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 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c429287dfbe

Comment 5

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/3c429287dfbe

Comment 6

[Mike Hommey [:glandium]]

Jan, I suspect this broke ia64 (actually, I'm pretty sure it did). Could you check?