Static strings should be GC things

RESOLVED FIXED in mozilla9

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
One example of such a bug is Bug 650519.
OS: Linux → All
Hardware: x86 → All
Posted patch patchSplinter Review
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.
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #558006 - Flags: review?(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?
Attachment #558006 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/3c429287dfbe
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Jan, I suspect this broke ia64 (actually, I'm pretty sure it did). Could you check?
You need to log in before you can comment on or make changes to this bug.