TM: Allocate short strings in the GC heap, avoiding malloc + free

RESOLVED DUPLICATE of bug 578205

Status

()

Core
JavaScript Engine
RESOLVED DUPLICATE of bug 578205
8 years ago
7 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

8 years ago
Late nate hacking with dvander. The attached patch creates a new type of GC thing: ShortStrings. Short strings are twice as large as sizeof(JSString), and we store their content in the 2nd half. Relevant frequent engine paths (such as number -> string for crying out loud) are intercepted and do not go into JS_OMG_ReallySlowAPICall. Instead, they get redirected into js_NewStringCopyZ/N, which does optimized string creation for both, char* and jschar*. This should probably also be inlined.
(Assignee)

Comment 1

8 years ago
Created attachment 433528 [details] [diff] [review]
patch
Assignee: general → gal
(Assignee)

Comment 2

8 years ago
Created attachment 433529 [details] [diff] [review]
patch
Attachment #433528 - Attachment is obsolete: true
(Assignee)

Comment 3

8 years ago
Created attachment 433530 [details] [diff] [review]
patch

Fixed a couple bugs. Properly finalize the short string arena.

Testcase:

var a = [];
for (var i = 0; i < 100000; ++i)
    a[i] = 0;

var t = Date.now();
for (var j = 0; j < 50; ++j)
    for (i in a)
	;
print(Date.now() - t);

Before: 2198ms
After: 1753ms

Note: I only patched num -> str conversion. There is probably other wins to be had that currently still go through JSAPI instead of js_NewStringCopyZ/N.
Attachment #433529 - Attachment is obsolete: true
Bug 402614 is on your list, still open -- why file a new bug?

/be
(Assignee)

Comment 5

8 years ago
About 1% speedup for SS. Its noisy as usual, but all the string stuff clearly benefitted (all the string test cases are significant).

TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           1.010x as fast    765.1ms +/- 0.3%   757.8ms +/- 0.3%     significant

=============================================================================

  3d:                  1.011x as fast    127.4ms +/- 0.7%   125.9ms +/- 0.5%     significant
    cube:              1.018x as fast     41.7ms +/- 1.0%    41.0ms +/- 1.2%     significant
    morph:             -                  23.0ms +/- 1.1%    22.8ms +/- 0.8% 
    raytrace:          -                  62.6ms +/- 0.6%    62.2ms +/- 0.3% 

  access:              -                 104.9ms +/- 0.5%   104.5ms +/- 0.3% 
    binary-trees:      *1.015x as slow*   21.5ms +/- 0.8%    21.8ms +/- 0.5%     significant
    fannkuch:          -                  49.6ms +/- 0.7%    49.5ms +/- 0.4% 
    nbody:             1.021x as fast     20.9ms +/- 0.7%    20.4ms +/- 0.7%     significant
    nsieve:            -                  13.0ms +/- 1.2%    12.8ms +/- 1.0% 

  bitops:              -                  33.4ms +/- 1.3%    33.2ms +/- 1.1% 
    3bit-bits-in-byte: -                   1.4ms +/- 10.1%     1.3ms +/- 10.2% 
    bits-in-byte:      -                  10.5ms +/- 2.6%    10.2ms +/- 1.9% 
    bitwise-and:       -                   2.3ms +/- 5.7%     2.2ms +/- 5.4% 
    nsieve-bits:       ??                 19.3ms +/- 0.8%    19.4ms +/- 0.9%     not conclusive: might be *1.008x as slow*

  controlflow:         ??                  8.1ms +/- 1.6%     8.3ms +/- 2.7%     not conclusive: might be *1.015x as slow*
    recursive:         ??                  8.1ms +/- 1.6%     8.3ms +/- 2.7%     not conclusive: might be *1.015x as slow*

  crypto:              ??                 49.8ms +/- 0.9%    50.0ms +/- 0.9%     not conclusive: might be *1.004x as slow*
    aes:               ??                 29.3ms +/- 1.3%    29.3ms +/- 1.4%     not conclusive: might be *1.001x as slow*
    md5:               ??                 13.9ms +/- 0.8%    14.0ms +/- 1.0%     not conclusive: might be *1.007x as slow*
    sha1:              ??                  6.7ms +/- 2.5%     6.8ms +/- 1.8%     not conclusive: might be *1.009x as slow*

  date:                ??                105.9ms +/- 0.5%   106.5ms +/- 0.4%     not conclusive: might be *1.005x as slow*
    format-tofte:      *1.015x as slow*   54.1ms +/- 0.5%    54.9ms +/- 0.5%     significant
    format-xparb:      -                  51.8ms +/- 0.6%    51.6ms +/- 0.5% 

  math:                ??                 35.6ms +/- 1.0%    35.8ms +/- 1.5%     not conclusive: might be *1.004x as slow*
    cordic:            -                  17.5ms +/- 1.4%    17.4ms +/- 1.7% 
    partial-sums:      ??                 11.8ms +/- 1.0%    11.9ms +/- 1.7%     not conclusive: might be *1.012x as slow*
    spectral-norm:     ??                  6.4ms +/- 2.2%     6.5ms +/- 2.2%     not conclusive: might be *1.012x as slow*

  regexp:              -                  39.7ms +/- 0.7%    39.5ms +/- 0.4% 
    dna:               -                  39.7ms +/- 0.7%    39.5ms +/- 0.4% 

  string:              1.024x as fast    260.2ms +/- 0.3%   254.1ms +/- 0.4%     significant
    base64:            1.050x as fast     11.0ms +/- 0.4%    10.5ms +/- 1.5%     significant
    fasta:             1.010x as fast     49.9ms +/- 0.7%    49.4ms +/- 0.4%     significant
    tagcloud:          1.018x as fast     89.7ms +/- 0.5%    88.1ms +/- 0.8%     significant
    unpack-code:       1.030x as fast     85.5ms +/- 0.3%    83.0ms +/- 0.4%     significant
    validate-input:    1.043x as fast     24.1ms +/- 0.6%    23.1ms +/- 0.5%     significant
(Assignee)

Comment 6

8 years ago
Do you want me to data mine bugzilla for old bugs I can't find, or make us faster? :)
(Assignee)

Updated

8 years ago
Blocks: 402614
(Assignee)

Comment 7

8 years ago
The speedup in the browser is likely higher. My money is on 2x over the shell numbers above (locking cost is especially high for small malloc sizes). Anyone care to measure?
(Assignee)

Comment 8

8 years ago
A couple additional notes regarding the profile of the test code:

- We spend 18.7% in mmap/munmap. Gregor's delayed chunk deallocation patch fixes this. I think this underlines just how urgently we need to apply that patch. Note that for ShortStrings there is no finalization. So moving finalization to a background thread buys us exactly nothing here.
- Walking the freelist of pages that are completely free takes 80% of GC time. My anything-life-on-this-page patch would really help here. Patch is pending review. Objects might be longer-lived often, but strings usually die young, increasing the chance of empty string pages.
- The num -> string path contains a number of expensive early exits and .cpp -> .cpp -> .cpp transitions (jsnum.cpp to jsapi.cpp to jsstr.cpp to jsgc.cpp). There are a few very trivial wins here by inlining better.
(Assignee)

Comment 9

8 years ago
Another shark note: recovering length with strlen() is expensive, at least for this testcase. We want to pipe the string length from IntToCString into NewStringCopyN instead of using NewStringCopyZ.
(In reply to comment #6)
> Do you want me to data mine bugzilla for old bugs I can't find, or make us
> faster? :)

Dude, just look at your own bugs.

Bugzilla is not a write-only medium.

/be
(Assignee)

Updated

8 years ago
Summary: TM: short strings, not sucky → TM: Allocate short strings in the GC heap, avoiding malloc + free
(Assignee)

Comment 11

8 years ago
Its a feature now. The other bug is a meta bug with gregor's numbers since there is still more to do with those.
(In reply to comment #8)
> My anything-live-on-this-page patch would really help here. Patch is pending
> review.

Bug #?

> - The num -> string path contains a number of expensive early exits and .cpp ->
> .cpp -> .cpp transitions (jsnum.cpp to jsapi.cpp to jsstr.cpp to jsgc.cpp).
> There are a few very trivial wins here by inlining better.

Internals are supposed to avoid JS_* calls, preferring non-declspec js_* faster internal APIs (now better named js::Whatever). Probably time to search for all such calls and fix them.

/be
Comment on attachment 433530 [details] [diff] [review]
patch

>+FitsIntoShortString(size_t length)
>+{
>+    return sizeof(jschar) * (length + 1) < sizeof(JSString);

Off-by-one fencepost bug: should use <= not < -- or, to avoid the + 1, just

     return sizeof(jschar) * length < sizeof(JSString).

/be
(Assignee)

Comment 14

8 years ago
bug 550373
(Assignee)

Comment 15

8 years ago
Its 2:30am, my brain is giving out on me, but I think the code is correct. FitsIntoShortString is given the string length, it needs that plus 1 character for the zero termination. That's the + 1. Note that I turn CopyZ into CopyN(strlen(s)). So the +1 makes sure we put the zero back behind it.
You want <= if you use length + 1. But that costs (source if not compiled code -- may be optimized away) so use < and lose the + 1, just use length. Since jschar size divides JSString size exactly, using < means you'll have one more jschar's worth of bytes to store the NUL, at least.

/be
(Assignee)

Comment 17

8 years ago
Every C/C++ compiler worth a penny optimizes + 1 < into <= for you if that's faster. Let's not try to outsmart compiler-based strength reduction. I really prefer readable code over that.

Example: string length = 7

FitsIntoShortString(7)

  (7 + 1) * 2 = 16, 16 < 16 => false

should be true, so (length + 1) <= sizeof or 

 7 * 2 < 16 => true

length * 2 < sizeof, as you said. I stand corrected on this one.
(Assignee)

Comment 18

8 years ago
Created attachment 433534 [details] [diff] [review]
patch, there is room for one more character
Attachment #433530 - Attachment is obsolete: true
(In reply to comment #17)
> Every C/C++ compiler worth a penny optimizes + 1 < into <= for you if that's
> faster. Let's not try to outsmart compiler-based strength reduction. I really
> prefer readable code over that.

Is it readable to use the more complicated expression? I don't think so. But it is up to you -- provided you check the compiled code. I don't trust gcc (ok, Mac gcc :-P).

/be
(Assignee)

Updated

8 years ago
Attachment #433534 - Flags: review?(igor)
(Assignee)

Comment 20

8 years ago
Ah crap. You are right. Retardo gcc doesn't optimize this it seems.

void a(int x, int y) {
  if (x + 1 <= y)
    z = 1;
}

LFB2:
	pushq	%rbp
LCFI0:
	movq	%rsp, %rbp
LCFI1:
	incl	%edi
	cmpl	%esi, %edi
	jg	L4
	movq	_z@GOTPCREL(%rip), %rax
	movl	$1, (%rax)
L4:
	leave
	ret

I am just going to shut up now :)
(Assignee)

Comment 21

8 years ago
If anyone wants to steal this bug, be my guest. Otherwise I will land it once it gets review.
(In reply to comment #20)
> Ah crap. You are right. Retardo gcc doesn't optimize this it seems.
> 
> void a(int x, int y) {

Try unsigned -- may succeed (and your JSShortString code uses unsigned integer types, so it would work too).

/be
(Assignee)

Comment 23

8 years ago
Nope.

LFB2:
	pushq	%rbp
LCFI0:
	movq	%rsp, %rbp
LCFI1:
	incl	%edi
	cmpl	%esi, %edi
	ja	L4
	movq	_z@GOTPCREL(%rip), %rax
	movl	$1, (%rax)
L4:
	leave
	ret

Comment 24

8 years ago
> inline void
>+FinalizeShortString(JSContext *cx, JSShortString *str, unsigned thingKind)
>+{
>+    JS_ASSERT(FINALIZE_SHORT_STRING == thingKind);
>+    JS_ASSERT(!JSString::isStatic(&str->header));
>+    JS_RUNTIME_UNMETER(cx->runtime, liveStrings);
>+    if (str->header.isDependent()) {

The patch should update js_ConcatStrings to make sure that short string chars are not reallocated. Also perhaps we should add an internal API to allocate a string together with buffer and let the caller to fill the buffer with characters later. This way we can replace a common pattern like (see http://hg.mozilla.org/tracemonkey/file/35030f4db298/js/src/jsparse.cpp#l9212 ): 

chars = (jschar *) cx->malloc((length + 1) * sizeof(jschar));
if (!chars)
    return JS_FALSE;
str = js_NewString(cx, chars, length);
if (!str) {
    cx->free(chars);
    return JS_FALSE;
}

with:

str = js_NewStringAndChars(cx, length);
if (!str)
    return false;
chars = str->chars();

Comment 25

8 years ago
Comment on attachment 433534 [details] [diff] [review]
patch, there is room for one more character

> inline void
>+FinalizeShortString(JSContext *cx, JSShortString *str, unsigned thingKind)
>+{
>+    JS_ASSERT(FINALIZE_SHORT_STRING == thingKind);
>+    JS_ASSERT(!JSString::isStatic(&str->header));
>+    JS_RUNTIME_UNMETER(cx->runtime, liveStrings);
>+    if (str->header.isDependent()) {

Have you checked that the compiler can truly eliminate the FinalizeShortString call? 

Also this should be reconsidered after updating js_ConcatStrings. On the first glance it does not make sense to make a dependent string that points into the short string there. 

>diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp
>--- a/js/src/jsstr.cpp
>+++ b/js/src/jsstr.cpp
>+bool
>+FitsIntoShortString(size_t length)
>+{
>+    return sizeof(jschar) * length < sizeof(JSString);
>+}

A possible overflow may prevent the compiler to optimize this as length < sizeof(JSString) / 2.So add a const like:

const size_t JS_SHORT_STRING_MAX_LENGTH = 4 * sizeof(jsuword) / sizeof(jschar) - 1 and use it here.

and use it here.

>+JSString *
>+NewShortString(JSContext *cx, const char *chars, size_t length)
>+{
...
>+    while (n-- > 0)
>+        *p++ = jschar(*chars++);

This does not respect js_CStringsAreUTF8.
Attachment #433534 - Flags: review?(igor) → review-
(Assignee)

Comment 26

8 years ago
I noticed js_CStringsAreUTF8. We don't seem to use that and its a fair amount of ugliness and complexity in the code. I would like to delete it. Any objections?
(In reply to comment #26)
> I noticed js_CStringsAreUTF8. We don't seem to use that and its a fair amount
> of ugliness and complexity in the code. I would like to delete it. Any
> objections?

Yes, it should not be deleted. CouchDB uses it, and more important to selfish us, we might really want to use it too. Do the work to support it, it's no more to be broken than js_ConcatStrings.

/be
I've started using js::Foo instead of js_Foo, FWIW. There's a bug on global js_ -> js:: conversion so it can wait but sometimes you're only adding code, and when adding code, why add more js_ C-style veneer?

/be
(Assignee)

Comment 29

8 years ago
(In reply to comment #24)
> > inline void
> >+FinalizeShortString(JSContext *cx, JSShortString *str, unsigned thingKind)
> >+{
> >+    JS_ASSERT(FINALIZE_SHORT_STRING == thingKind);
> >+    JS_ASSERT(!JSString::isStatic(&str->header));
> >+    JS_RUNTIME_UNMETER(cx->runtime, liveStrings);
> >+    if (str->header.isDependent()) {
> 
> The patch should update js_ConcatStrings to make sure that short string chars
> are not reallocated. Also perhaps we should add an internal API to allocate a

I never set the MUTABLE flag, so my short strings should never be grown or modified.

> string together with buffer and let the caller to fill the buffer with
> characters later. This way we can replace a common pattern like (see
> http://hg.mozilla.org/tracemonkey/file/35030f4db298/js/src/jsparse.cpp#l9212 ): 

Not opposed to a separate cleanup bug but I would like to keep this patch short and to the point.

> 
> chars = (jschar *) cx->malloc((length + 1) * sizeof(jschar));
> if (!chars)
>     return JS_FALSE;
> str = js_NewString(cx, chars, length);
> if (!str) {
>     cx->free(chars);
>     return JS_FALSE;
> }
> 
> with:
> 
> str = js_NewStringAndChars(cx, length);
> if (!str)
>     return false;
> chars = str->chars();
(Assignee)

Comment 30

8 years ago
(In reply to comment #25)
> (From update of attachment 433534 [details] [diff] [review])
> > inline void
> >+FinalizeShortString(JSContext *cx, JSShortString *str, unsigned thingKind)
> >+{
> >+    JS_ASSERT(FINALIZE_SHORT_STRING == thingKind);
> >+    JS_ASSERT(!JSString::isStatic(&str->header));
> >+    JS_RUNTIME_UNMETER(cx->runtime, liveStrings);
> >+    if (str->header.isDependent()) {
> 
> Have you checked that the compiler can truly eliminate the FinalizeShortString
> call? 

Done. No code is generated in an opt build with gcc -O3.

> 
> Also this should be reconsidered after updating js_ConcatStrings. On the first
> glance it does not make sense to make a dependent string that points into the
> short string there.

Why not?

> 
> >diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp
> >--- a/js/src/jsstr.cpp
> >+++ b/js/src/jsstr.cpp
> >+bool
> >+FitsIntoShortString(size_t length)
> >+{
> >+    return sizeof(jschar) * length < sizeof(JSString);
> >+}
> 
> A possible overflow may prevent the compiler to optimize this as length <
> sizeof(JSString) / 2.So add a const like:
> 
> const size_t JS_SHORT_STRING_MAX_LENGTH = 4 * sizeof(jsuword) / sizeof(jschar)
> - 1 and use it here.
> 
> and use it here.

Good point. I moved the constant multiplication to the other size of the expression and checked the compiler output. The declaration seems overkill since its used in one place only.

> 
> >+JSString *
> >+NewShortString(JSContext *cx, const char *chars, size_t length)
> >+{
> ...
> >+    while (n-- > 0)
> >+        *p++ = jschar(*chars++);
> 
> This does not respect js_CStringsAreUTF8.

I disabled short strings for UTF8 encoding.
(Assignee)

Comment 31

8 years ago
Created attachment 433666 [details] [diff] [review]
patch
Attachment #433534 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #433666 - Flags: review?(igor)

Comment 32

8 years ago
(In reply to comment #31)
> Created an attachment (id=433666) [details]
> patch

Hm, this looks like the old patch.

(In reply to comment #30)
> > Have you checked that the compiler can truly eliminate the FinalizeShortString
> > call? 
> 
> Done. No code is generated in an opt build with gcc -O3.

Since ShortString does not have a finalizer, it should be treated as double. For them the code assembles the free list during allocation skipping the finalization completely. But that can be done in a separated bug.

> > This does not respect js_CStringsAreUTF8.
> 
> I disabled short strings for UTF8 encoding.

What is wrong with calling js_InflateStringToBuffer couple of times first to measure the size and then to decode following the pattern in js_InflateString? Also in many cases the call can be made just once since for UTF8->UTF16 conversion the number of 2-byte jschars is less or equal to the number of bytes in the original string. Thus a quick check would see if the bytes fits into the short buffer. 

Yet another observation is that the patch can fit 2 more chars into the short string. JSString::mOffset is used only for dependent strings. Thus, if the field would be the last in JSString, the short string chars can starts there.

Comment 33

8 years ago
(In reply to comment #32)
> Hm, this looks like the old patch.

Sorry, I have opened the wrong attachment. Still the above comments applies.
(Assignee)

Comment 34

8 years ago
> Since ShortString does not have a finalizer, it should be treated as double.
> For them the code assembles the free list during allocation skipping the
> finalization completely. But that can be done in a separated bug.

Sure. I will file a bug.

> What is wrong with calling js_InflateStringToBuffer couple of times first to
> measure the size and then to decode following the pattern in js_InflateString?
> Also in many cases the call can be made just once since for UTF8->UTF16
> conversion the number of 2-byte jschars is less or equal to the number of bytes
> in the original string. Thus a quick check would see if the bytes fits into the
> short buffer.

We don't use UTF8. I don't want to use company time and resources to optimize for a corner case we don't use. Note that this doesn't break functionality for anyone. We merely optimize less in a configuration we don't use. I will file a bug to make UTF8 a pure compile-time option. If other embedders want to use short strings from utf8 sources, and it doesn't slow us down (not 1 cycle), we should take a patch.

> Yet another observation is that the patch can fit 2 more chars into the short
> string. JSString::mOffset is used only for dependent strings. Thus, if the
> field would be the last in JSString, the short string chars can starts there.

I am not sure this provides a measurable speedup, and this is a bit messy and complex. If you want to file a bug I won't protest, but I don't think the basic patch needs this.
(Assignee)

Comment 35

8 years ago
filed bug 553780 for the finalization improvement

Comment 36

8 years ago
(In reply to comment #34)
> We don't use UTF8. I don't want to use company time and resources to optimize
> for a corner case we don't use. Note that this doesn't break functionality for
> anyone. We merely optimize less in a configuration we don't use. I will file a
> bug to make UTF8 a pure compile-time option. If other embedders want to use
> short strings from utf8 sources, and it doesn't slow us down (not 1 cycle), we
> should take a patch.

This is a bug in XPConnect that it does not use this option. Clearly this is a low priority but that does not mean that one day that bug would be fixed. In that case somebody may spend some time figuring out why that patch made some things slower. 

Given that however theoretical possibility and the fact that some time has already been spend on not supporting that option and those efforts lead to a bug (FitsIntoShortString checks for js_CStringsAreUTF8 meaning that the runtime check would present even for pure jschar operations), I really suggest just write the proper support.
I agree with Igor it would take less time to do UTF-8 right than argue about it.

I think Andreas and I agree that UTF-8 could be a compile time option again (he and I talked about this, I looked at the CVS history, which led to bug 397215 -- see bug 397215 comment 7 and mrbkap's later bug 397215 comment 14 which proposed the runtime check used when JS_C_STRINGS_ARE_UTF8 is defined.

But thinking about it more, considering how XPConnect has to enforce ASCII-only char strings in and out of the JS engine, it seems to me we should aim higher.

We have the unified build system wanted by downstreams (thanks to jimb). We do not yet do the two-way UTF8/not-UTF8 testing. We do have CouchDB embedding us, and pretty fresh source, IIRC.

Compile-time options are bad, they inevitably mean more testing and likely gaps in testing all the #ifdef combos.

For stronger reasons than this, but also for this reason, we want to get rid of JS_THREADSAFE by going single-threaded with the request model kept for the foreseeable future, but no ownercx and GC lock costs.

Can we eliminate the JS_C_STRINGS_ARE_UTF8 #ifdefs and make the runtime checking efficient enough?

Or could we even just switch to C-strings-are-UTF8 now that we have the Encode/Decode API for dealing with UTF-8 chars, vs. the old API that can be used to hack raw bytes (or ISO-Latin-1 code points) into JS strings by zero-padding them on the left?

I'm not looking to make more work here and now. We can find a way forward, e.g. by not allowing UTF-8 into small strings. I'm looking for some followup bugs, a roadmap, the big picture.

/be
(Assignee)

Comment 38

8 years ago
There _is_ proper support for it. With this patch, we behave _exactly_ the way as before. Nothing breaks. All I tried to avoid is spending time I could do something else optimizing a case we don't use. There is no evidence CouchDB uses as many short strings as we do, and actually benefits from this. If they do, we can always optimize later (ideally they can and then they contribute it). Profile driven optimization and all. But anyway, I agree that discussing this is consuming already more time than optimizing that corner case, so I will submit a new patch.
(Assignee)

Comment 39

8 years ago
Created attachment 433745 [details] [diff] [review]
patch
Attachment #433666 - Attachment is obsolete: true
Attachment #433666 - Flags: review?(igor)
(Assignee)

Updated

8 years ago
Attachment #433745 - Flags: review?(igor)

Comment 40

8 years ago
Comment on attachment 433745 [details] [diff] [review]
patch

>diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp
>+static bool
>+FitsIntoShortString(size_t length)
>+{
>+    return length < (sizeof(JSString) / sizeof(jschar));

Nit: redundant ().

> static const size_t sMinWasteSize = 16;
> 
> JSString *
> js_NewStringFromCharBuffer(JSContext *cx, JSCharBuffer &cb)
> {
>     if (cb.empty())
>         return ATOM_TO_STRING(cx->runtime->atomState.emptyAtom);
> 
>@@ -3199,49 +3242,70 @@ void printJSStringStats(JSRuntime *rt)
>     fprintf(stderr, "%lu total dependent strings, mean length %g (sigma %g)\n",
>             (unsigned long)rt->totalDependentStrings, mean, sigma);
> }
> #endif
> 
> JSString *
> js_NewStringCopyN(JSContext *cx, const jschar *s, size_t n)
> {
>+    if (FitsIntoShortString(n))
>+        return NewShortString(cx, s, n);
>+
>     jschar *news;
>     JSString *str;
> 
>     news = (jschar *) cx->malloc((n + 1) * sizeof(jschar));
>     if (!news)
>         return NULL;
>     js_strncpy(news, s, n);
>     news[n] = 0;
>     str = js_NewString(cx, news, n);
>     if (!str)
>         cx->free(news);
>     return str;
> }
> 
> JSString *
>+js_NewStringCopyN(JSContext *cx, const char *s, size_t n)
>+{

Nit: comment here that for code simplicity with UTF-8 enabled the code uses conservatively that the length of decoded string is no more than the number of bytes.
Attachment #433745 - Flags: review?(igor) → review+

Updated

8 years ago
Blocks: 553824

Comment 41

8 years ago
Comment on attachment 433745 [details] [diff] [review]
patch

>diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp
>+++ b/js/src/jsgc.cpp
>@@ -752,16 +752,17 @@ GetFinalizableThingSize(unsigned thingKi
>     JS_STATIC_ASSERT(JS_EXTERNAL_STRING_LIMIT == 8);
> 
>     static const uint8 map[FINALIZE_LIMIT] = {
>         sizeof(JSObject),   /* FINALIZE_OBJECT */
>         sizeof(JSFunction), /* FINALIZE_FUNCTION */
> #if JS_HAS_XML_SUPPORT
>         sizeof(JSXML),      /* FINALIZE_XML */
> #endif
>+        sizeof(JSShortString), /* FINALIZE_SHORT_STRING  */
>         sizeof(JSString),   /* FINALIZE_STRING */
>         sizeof(JSString),   /* FINALIZE_EXTERNAL_STRING0 */
>         sizeof(JSString),   /* FINALIZE_EXTERNAL_STRING1 */
>         sizeof(JSString),   /* FINALIZE_EXTERNAL_STRING2 */
>         sizeof(JSString),   /* FINALIZE_EXTERNAL_STRING3 */
>         sizeof(JSString),   /* FINALIZE_EXTERNAL_STRING4 */
>         sizeof(JSString),   /* FINALIZE_EXTERNAL_STRING5 */
>         sizeof(JSString),   /* FINALIZE_EXTERNAL_STRING6 */
>@@ -775,20 +776,21 @@ GetFinalizableThingSize(unsigned thingKi
> static inline size_t
> GetFinalizableTraceKind(size_t thingKind)
> {
>     JS_STATIC_ASSERT(JS_EXTERNAL_STRING_LIMIT == 8);
> 
>     static const uint8 map[FINALIZE_LIMIT] = {
>         JSTRACE_OBJECT,     /* FINALIZE_OBJECT */
>         JSTRACE_OBJECT,     /* FINALIZE_FUNCTION */
>-#if JS_HAS_XML_SUPPORT      /* FINALIZE_XML */
>-        JSTRACE_XML,
>-#endif                      /* FINALIZE_STRING */
>-        JSTRACE_STRING,
>+#if JS_HAS_XML_SUPPORT
>+        JSTRACE_XML,        /* FINALIZE_XML */
>+#endif
>+        JSTRACE_STRING,     /* FINALIZE_SHORT_STRING     */
>+        JSTRACE_STRING,     /* FINALIZE_STRING           */
>         JSTRACE_STRING,     /* FINALIZE_EXTERNAL_STRING0 */
>         JSTRACE_STRING,     /* FINALIZE_EXTERNAL_STRING1 */
>         JSTRACE_STRING,     /* FINALIZE_EXTERNAL_STRING2 */
>         JSTRACE_STRING,     /* FINALIZE_EXTERNAL_STRING3 */
>         JSTRACE_STRING,     /* FINALIZE_EXTERNAL_STRING4 */
>         JSTRACE_STRING,     /* FINALIZE_EXTERNAL_STRING5 */
>         JSTRACE_STRING,     /* FINALIZE_EXTERNAL_STRING6 */
>         JSTRACE_STRING,     /* FINALIZE_EXTERNAL_STRING7 */
>@@ -861,17 +863,17 @@ js_GetGCThingTraceKind(void *thing)
> 
> JSRuntime*
> js_GetGCStringRuntime(JSString *str)
> {
>     JSGCArenaList *list = JSGCArena::fromGCThing(str)->info.list;
>     JS_ASSERT(list->thingSize == sizeof(JSString));
> 
>     unsigned i = list->thingKind;
>-    JS_ASSERT(i == FINALIZE_STRING ||
>+    JS_ASSERT(i == FINALIZE_STRING || i == FINALIZE_SHORT_STRING ||
>               (FINALIZE_EXTERNAL_STRING0 <= i &&
>                i < FINALIZE_EXTERNAL_STRING0 + JS_EXTERNAL_STRING_LIMIT));
>     return (JSRuntime *)((uint8 *)(list - i) -
>                          offsetof(JSRuntime, gcArenaList));
> }
> 
> bool
> js_IsAboutToBeFinalized(void *thing)
>@@ -980,16 +982,17 @@ js_DumpGCStats(JSRuntime *rt, FILE *fp)
> {
>     static const char *const GC_ARENA_NAMES[] = {
>         "double",
>         "object",
>         "function",
> #if JS_HAS_XML_SUPPORT
>         "xml",
> #endif
>+        "short_string",
>         "string",
>         "external_string_0",
>         "external_string_1",
>         "external_string_2",
>         "external_string_3",
>         "external_string_4",
>         "external_string_5",
>         "external_string_6",
>@@ -2329,16 +2332,17 @@ JSWeakRoots::mark(JSTracer *trc)
> {
> #ifdef DEBUG
>     const char * const newbornNames[] = {
>         "newborn_object",             /* FINALIZE_OBJECT */
>         "newborn_function",           /* FINALIZE_FUNCTION */
> #if JS_HAS_XML_SUPPORT
>         "newborn_xml",                /* FINALIZE_XML */
> #endif
>+        "newborn_short_string",       /* FINALIZE_SHORT_STRING */
>         "newborn_string",             /* FINALIZE_STRING */
>         "newborn_external_string0",   /* FINALIZE_EXTERNAL_STRING0 */
>         "newborn_external_string1",   /* FINALIZE_EXTERNAL_STRING1 */
>         "newborn_external_string2",   /* FINALIZE_EXTERNAL_STRING2 */
>         "newborn_external_string3",   /* FINALIZE_EXTERNAL_STRING3 */
>         "newborn_external_string4",   /* FINALIZE_EXTERNAL_STRING4 */
>         "newborn_external_string5",   /* FINALIZE_EXTERNAL_STRING5 */
>         "newborn_external_string6",   /* FINALIZE_EXTERNAL_STRING6 */
>@@ -2602,16 +2606,28 @@ js_ChangeExternalStringFinalizer(JSStrin
>             str_finalizers[i] = newop;
>             return intN(i);
>         }
>     }
>     return -1;
> }
> 
> inline void
>+FinalizeShortString(JSContext *cx, JSShortString *str, unsigned thingKind)
>+{
>+    JS_ASSERT(FINALIZE_SHORT_STRING == thingKind);
>+    JS_ASSERT(!JSString::isStatic(&str->header));
>+    JS_RUNTIME_UNMETER(cx->runtime, liveStrings);
>+    if (str->header.isDependent()) {
>+        JS_ASSERT(str->header.dependentBase());
>+        JS_RUNTIME_UNMETER(cx->runtime, liveDependentStrings);
>+    }
>+}
>+
>+inline void
> FinalizeString(JSContext *cx, JSString *str, unsigned thingKind)
> {
>     JS_ASSERT(FINALIZE_STRING == thingKind);
>     JS_ASSERT(!JSString::isStatic(str));
>     JS_RUNTIME_UNMETER(cx->runtime, liveStrings);
>     if (str->isDependent()) {
>         JS_ASSERT(str->dependentBase());
>         JS_RUNTIME_UNMETER(cx->runtime, liveDependentStrings);
>@@ -2657,16 +2673,17 @@ js_FinalizeStringRT(JSRuntime *rt, JSStr
>         /* A dependent string can not be external and must be valid. */
>         JS_ASSERT(JSGCArena::fromGCThing(str)->info.list->thingKind ==
>                   FINALIZE_STRING);
>         JS_ASSERT(str->dependentBase());
>         JS_RUNTIME_UNMETER(rt, liveDependentStrings);
>     } else {
>         unsigned thingKind = JSGCArena::fromGCThing(str)->info.list->thingKind;
>         JS_ASSERT(IsFinalizableStringKind(thingKind));
>+        JS_ASSERT(thingKind != FINALIZE_SHORT_STRING);
> 
>         /* A stillborn string has null chars, so is not valid. */
>         jschar *chars = str->flatChars();
>         if (!chars)
>             return;
>         if (thingKind == FINALIZE_STRING) {
>             rt->free(chars);
>         } else {
>@@ -3191,16 +3208,18 @@ js_GC(JSContext *cx, JSGCInvocationKind 
> #endif
> 
>     /*
>      * We sweep the deflated cache before we finalize the strings so the
>      * cache can safely use js_IsAboutToBeFinalized..
>      */
>     rt->deflatedStringCache->sweep(cx);
> 
>+    FinalizeArenaList<JSShortString, FinalizeShortString>
>+        (cx, FINALIZE_SHORT_STRING, &emptyArenas);
>     FinalizeArenaList<JSString, FinalizeString>
>         (cx, FINALIZE_STRING, &emptyArenas);
>     for (unsigned i = FINALIZE_EXTERNAL_STRING0;
>          i <= FINALIZE_EXTERNAL_STRING_LAST;
>          ++i) {
>         FinalizeArenaList<JSString, FinalizeExternalString>
>             (cx, i, &emptyArenas);
>     }
>diff --git a/js/src/jsgc.h b/js/src/jsgc.h
>--- a/js/src/jsgc.h
>+++ b/js/src/jsgc.h

>+struct JSShortString;

One more nit: move this into jsprvtd.h
Nit-picking Igor's final nit:

We don't need typedefs outside of the public C-or-C++ API any longer -- C++ makes the struct tag the typename. We do need forward or opaque typename declarations, however. So I don't see why that struct JSShortString needs to be in jsprvtd.h. It could be introduced only where it is actually needed in other .h files that should not include jsstr.h.

/be
(Assignee)

Comment 43

8 years ago
http://hg.mozilla.org/tracemonkey/rev/1dca6e135a1e
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 44

8 years ago
and now brand-new and with more parens

http://hg.mozilla.org/tracemonkey/rev/400097fb04e7
(Assignee)

Comment 45

8 years ago
for the record, this built just fine under macosx

http://hg.mozilla.org/tracemonkey/rev/38cb39bc6744
(Assignee)

Comment 46

8 years ago
Waldo backed this out. However, it looks like the "orange" was really a "red" (build failure), which the last patch actually fixed. As usual, I can't make sense of our ever so weird tinderbox logs.

http://hg.mozilla.org/tracemonkey/rev/3383cccf276c
(Assignee)

Updated

8 years ago
Whiteboard: fixed-in-tracemonkey
(In reply to comment #40)
> (From update of attachment 433745 [details] [diff] [review])
> >diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp
> >+static bool
> >+FitsIntoShortString(size_t length)
> >+{
> >+    return length < (sizeof(JSString) / sizeof(jschar));
> 
> Nit: redundant ().

This nit was clearly about the outer () -- typenames require parens as typeof operands, a well-known C/C++ typename-is-not-a-lexical-identifier feature.

Testing before committing is always a good idea, and better than adding extra parens in advance and making a wish :-P.

http://tests.themasta.com/tinderboxpushlog/?tree=TraceMonkey pretty clearly shows widespread orange, not just red, even after the third commit.

Of course after Waldo's backout we have a couple of red (first one seems to be red due to "Error: failed graph server post") and some (much less widespread) orange still.

Is the tree just not in great metastable shape? After http://hg.mozilla.org/tracemonkey/rev/9ab9132b1055 (mrbkap's cset, last before the ones for this bug landed) it seemed pretty good.

/be
> Testing before committing is always a good idea, and better than adding extra

I meant "Test-building before committing ..." :-/.

/be
(In reply to comment #48)
> > Testing before committing is always a good idea, and better than adding extra
> 
> I meant "Test-building before committing ..." :-/.

Both, really.

Comment 50

8 years ago
http://hg.mozilla.org/mozilla-central/rev/1dca6e135a1e
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 51

8 years ago
(In reply to comment #50)
> http://hg.mozilla.org/mozilla-central/rev/1dca6e135a1e

How this can get merged with mozilla-central when the patch was backed out in TM tree???

Comment 52

8 years ago
oops sorry. here's the backout that got merged as well:

http://hg.mozilla.org/mozilla-central/rev/da8b6eaa2a30
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 402614

Updated

8 years ago
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 578205
No longer blocks: 553824
You need to log in before you can comment on or make changes to this bug.