Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Static strings should be GC things

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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
(Assignee)

Comment 2

6 years ago
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.
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #558006 - Flags: review?(luke)

Comment 3

6 years ago
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+
(Assignee)

Comment 4

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c429287dfbe
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/3c429287dfbe
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.