Jan de Mooij [:jandem] Assignee
	Description • 10 years ago

With my local patch queue we're down to < 20 jit-test failures with latin1 strings enabled. The remaining failures are in some pretty obscure corners of the engine exercised by 1 or 2 tests (the long tail).

	Jan de Mooij [:jandem] Assignee
	Comment 4 • 10 years ago

The Quote function used for JSON.stringify.

	Jeff Walden [:Waldo]
	Comment 7 • 10 years ago

Comment on attachment 8442778 [details] [diff] [review]
Part 3 - JS_EncodeString and friends

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

::: js/src/jsapi-tests/testUTF8.cpp
@@ +39,5 @@
>  
>  BEGIN_TEST(testUTF8_badSurrogate)
>  {
>      static const jschar badSurrogate[] = { 'A', 'B', 'C', 0xDEEE, 'D', 'E', 0 };
> +    const mozilla::Range<const jschar> tbchars(badSurrogate, js_strlen(badSurrogate));

It occurs to me you can't actually modify the pointer/length (pointer/end pointer), so tagging every Range as const isn't necessary, we could just make mStart/mEnd both const instead.  (And Range::operator[] should be const but return T&, so constness of elements is determined by element type.)  Don't care if you decide to not remove all these extra consts, but they wouldn't be necessary with this tweak.

::: js/src/jsapi.cpp
@@ +5513,5 @@
> +    if (linear->hasTwoByteChars())
> +        return JS::LossyTwoByteCharsToNewLatin1CharsZ(cx, linear->twoByteRange(nogc)).c_str();
> +
> +    size_t len = str->length();
> +    char *buf = cx->pod_malloc<char>(len + 1);

Latin1Char rather than char, seems.

@@ +5517,5 @@
> +    char *buf = cx->pod_malloc<char>(len + 1);
> +    if (!buf)
> +        return nullptr;
> +
> +    mozilla::PodCopy(reinterpret_cast<Latin1Char*>(buf), linear->latin1Chars(nogc), len);

Which kills off this cast.

@@ +5518,5 @@
> +    if (!buf)
> +        return nullptr;
> +
> +    mozilla::PodCopy(reinterpret_cast<Latin1Char*>(buf), linear->latin1Chars(nogc), len);
> +    buf[len] = 0;

Mild preference for '\0' for null terminators in strings.

	Jan de Mooij [:jandem] Assignee
	Comment 8 • 10 years ago

As discussed on IRC, this makes mStart and mEnd const and changes operator[] to be const and return non-const T&.

I can build the shell with this patch; will send to Try before landing to make sure it doesn't break something else.

	Jeff Walden [:Waldo]
	Comment 9 • 10 years ago

Comment on attachment 8442786 [details] [diff] [review]
Part 4 - Quote

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

::: js/src/json.cpp
@@ +121,5 @@
>  
> +static bool
> +Quote(JSContext *cx, StringBuffer &sb, JSString *str)
> +{
> +    JS::Anchor<JSString *> anchor(str);

This anchoring thing rather than handles is nonsense, but pre-existing.  Maybe I'll fix after you land your patches.

	Jeff Walden [:Waldo]
	Comment 10 • 10 years ago

Comment on attachment 8442863 [details] [diff] [review]
Part 7 - minor Range changes

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

Tryserver this before trusting it to land -- I would be entirely unsurprised to find someone with

Range x;
if (...)
  x = ...;
else
  x = ...;

in the whole browser.

	Luke Wagner [:luke]
	Comment 12 • 10 years ago

Comment on attachment 8442802 [details] [diff] [review]
Part 5 - FunctionToString

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

::: js/src/jsfun.cpp
@@ +808,5 @@
>          JS_ASSERT(!braced);
>          for (; unicode::IsSpaceOrBOM2(end[-1]); end--)
>              ;
>      }
> +    *bodyEnd = end.get() - srcChars.start().get();

I think you can subtract them directly w/o .get() (via operator- overload in RangedPtr).

	Luke Wagner [:luke]
	Comment 13 • 10 years ago

Comment on attachment 8442845 [details] [diff] [review]
Part 6 - asm.js PropertyName serialization

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

Thanks for doing this!

::: js/src/jit-test/tests/latin1/asmjs.js
@@ +1,1 @@
> +load(libdir + "asm.js");

Can you name this file asm.js?  The reason is that I often run 'jit_tests.py $(SHELL) asm.js' to run all the asm.js test.

::: js/src/jit/AsmJSModule.cpp
@@ +761,4 @@
>  {
> +    js::Vector<CharT> tmp(cx);
> +    CharT *src;
> +    if (sizeof(CharT) > 1 && (size_t(cursor) & (sizeof(CharT) - 1)) != 0) {

I think you don't need the "sizeof(CharT) > 1" conjunct: if sizeof(CharT) == 1, then the mask will be 0 and so the branch will not be taken.

	Jan de Mooij [:jandem] Assignee
	Comment 14 • 10 years ago

SavedFrame::HashPolicy::hash, SPSProfiler::allocProfileString, PrintHelpString and ShellSourceHook are all pretty straight-forward.

	Jan de Mooij [:jandem] Assignee
	Comment 16 • 10 years ago

This patch also changes AutoStableStringChars so that the string is passed to init() instead of to the constructor. This is nicer because init() can now call the (fallible) ensureLinear(), so that the callers don't have to flatten.

	Nicholas Nethercote [inactive]
	Comment 17 • 10 years ago

Comment on attachment 8442991 [details] [diff] [review]
Part 10 - ObjectToSource

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

::: js/src/jit-test/tests/latin1/assorted.js
@@ +15,5 @@
> +
> +// obj.toSource TwoByte
> +Object.defineProperty(o, "prop", {get: function() { return "\u1200"; },
> +				 set: function() { return "\u1200"; },
> +				 enumerable: true});

Nit: indent this like the one above.

	Jan de Mooij [:jandem] Assignee
	Comment 18 • 10 years ago

Parts 1-4:

https://hg.mozilla.org/integration/mozilla-inbound/rev/573aff4a3633
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4819ca6341
https://hg.mozilla.org/integration/mozilla-inbound/rev/21035d61ea24
https://hg.mozilla.org/integration/mozilla-inbound/rev/a03ae2d574c2

	Jan de Mooij [:jandem] Assignee
	Comment 19 • 10 years ago

Note that I'm using AutoStableStringChars more often in shell-only stuff and on error paths, as any work to avoid inflating isn't really worth it there.

	Jan de Mooij [:jandem] Assignee
	Comment 20 • 10 years ago

Parts 5, 6 and 8:

https://hg.mozilla.org/integration/mozilla-inbound/rev/03e66783afe6
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6db1bc87a22
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2db036eab81

	Jan de Mooij [:jandem] Assignee
	Comment 21 • 10 years ago

Some strings have "pure chars", which means the chars can be accessed without copying. We use this in JSCompartment::wrap, ScopedThreadSafeStringInspector and MemoryMetrics.

The pureCharsZ functions were not used at all, so I removed them.

After that, it turned out to be simpler/cleaner to use isLinear directly instead of hasPureChars, with a method on JSRope to copy the chars.

	Jan de Mooij [:jandem] Assignee
	Comment 22 • 10 years ago

Parts 9-11:

https://hg.mozilla.org/integration/mozilla-inbound/rev/24c04535d0e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/362efca784bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f37eeec2728

	Till Schneidereit [:till]
	Comment 24 • 10 years ago

Comment on attachment 8443475 [details] [diff] [review]
Part 14 - Self-hosting CloneString

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

I wonder if we even need support for twoByteChars here. I don't see why any self-hosting code should ever use non-latin1 chars. Maybe assert that and only handle latin1?

	Jan de Mooij [:jandem] Assignee
	Comment 26 • 10 years ago

(In reply to Till Schneidereit [:till] from comment #24)
> I wonder if we even need support for twoByteChars here. I don't see why any
> self-hosting code should ever use non-latin1 chars. Maybe assert that and
> only handle latin1?

Good point, but we need it for now because Latin1 strings are not yet enabled by default (I want to get these patches in before that happens). I'd be happy to file a follow-up bug to change it later on?

	Jan de Mooij [:jandem] Assignee
	Comment 27 • 10 years ago

And part 7:

https://hg.mozilla.org/integration/mozilla-inbound/rev/17cf9036f67f

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> Tryserver this before trusting it to land -- 

https://tbpl.mozilla.org/?tree=Try&rev=e98ed5c761e6

	Till Schneidereit [:till]
	Comment 28 • 10 years ago

Comment on attachment 8443475 [details] [diff] [review]
Part 14 - Self-hosting CloneString

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

Right, I didn't consider the piecemeal-landing. Makes total sense to do it this way for now and remove the twoByteChar stuff in a followup. If it's even useful to do it then, that is: it's not like this introduces terrible amounts of overhead or complexity.

	Ed Morley [:emorley]
	Comment 29 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/573aff4a3633
https://hg.mozilla.org/mozilla-central/rev/4c4819ca6341
https://hg.mozilla.org/mozilla-central/rev/21035d61ea24
https://hg.mozilla.org/mozilla-central/rev/a03ae2d574c2

	Jan de Mooij [:jandem] Assignee
	Comment 31 • 10 years ago

The shell's OffThreadCompileScript is a bit annoying because we have to make sure the chars are kept live until the off-thread compilation is done, so the AutoStableStringChars destructor should not free them.

	Jan de Mooij [:jandem] Assignee
	Comment 32 • 10 years ago

Not perf-sensitive so AutoStableStringChars will do.

	Jan de Mooij [:jandem] Assignee
	Comment 33 • 10 years ago

The previous patch stored the string length shifted by 1 and used the low bit for the isLatin1 bit. This is not backwards-compatible and that's complicated for IndexedDB, because we'd have to write a converter from v2 to v3 (or support both formats in the JS engine).

This patch just stores the isLatin1 bit in the high bit, without shifting the length, so that it's backwards compatible (v2 structured clone data is never Latin1 and will be read just fine), so that we don't have to worry about this.

bent, can you review the IndexedDB changes?

	Jan de Mooij [:jandem] Assignee
	Comment 34 • 10 years ago

Parts 12, 14, 16 and 17:

https://hg.mozilla.org/integration/mozilla-inbound/rev/691f218b408d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f31936a359a
https://hg.mozilla.org/integration/mozilla-inbound/rev/97cb97118cce
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae12316f60c1

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 36 • 10 years ago

Comment on attachment 8443897 [details] [diff] [review]
Part 15 - StructuredClone v2

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

r=me on the IDB changes

	Jan de Mooij [:jandem] Assignee
	Comment 37 • 10 years ago

With the patch in bug 1026438 and the patches in this bug jit-tests and jstests pass with Latin1 strings enabled.

	Jan de Mooij [:jandem] Assignee
	Comment 38 • 10 years ago

Part 13:

https://hg.mozilla.org/integration/mozilla-inbound/rev/08239aab0872

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/03e66783afe6
https://hg.mozilla.org/mozilla-central/rev/c6db1bc87a22
https://hg.mozilla.org/mozilla-central/rev/f2db036eab81
https://hg.mozilla.org/mozilla-central/rev/24c04535d0e0
https://hg.mozilla.org/mozilla-central/rev/362efca784bb
https://hg.mozilla.org/mozilla-central/rev/3f37eeec2728
https://hg.mozilla.org/mozilla-central/rev/17cf9036f67f
https://hg.mozilla.org/mozilla-central/rev/691f218b408d
https://hg.mozilla.org/mozilla-central/rev/8f31936a359a
https://hg.mozilla.org/mozilla-central/rev/97cb97118cce
https://hg.mozilla.org/mozilla-central/rev/ae12316f60c1

	Luke Wagner [:luke]
	Comment 40 • 10 years ago

Comment on attachment 8444317 [details] [diff] [review]
Part 20 - Almost everything else

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

::: js/src/jsstr.cpp
@@ +958,5 @@
>      }
>      if (U_FAILURE(status))
>          return false;
> +
> +    JSString *ns = js_NewStringCopyN<CanGC>(cx, chars.begin(), size);

IIUC, won't this introduce an extra malloc+copy in the case of a string bigger than INLINE_CAPACITY?

	Jan de Mooij [:jandem] Assignee
	Comment 41 • 10 years ago

(In reply to Luke Wagner [:luke] from comment #40)
> IIUC, won't this introduce an extra malloc+copy in the case of a string
> bigger than INLINE_CAPACITY?

Yeah, the alternative is to allow deflating StringBuffer (TwoByte => Latin1). We could add a StringBuffer::maybeDeflate method that we could call if the input is Latin1; I can post an updated patch to do this if you think that's fine.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 42 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/08239aab0872

	Jan de Mooij [:jandem] Assignee
	Comment 43 • 10 years ago

Parts 18-20:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c88b1d63a5f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c96053f6a716
https://hg.mozilla.org/integration/mozilla-inbound/rev/167b98c4bc86

	Jason Orendorff [:jorendorff]
	Comment 44 • 10 years ago

Comment on attachment 8443897 [details] [diff] [review]
Part 15 - StructuredClone v2

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

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +2116,5 @@
>            rv = UpgradeSchemaFrom13_0To14_0(connection);
>          }
> +        else if (schemaVersion == MakeSchemaVersion(14, 0)) {
> +          rv = UpgradeSchemaFrom14_0To15_0(connection);
> +        }

Huh. I didn't know how this stuff worked.

::: js/src/vm/StructuredClone.cpp
@@ +1205,5 @@
>      return str;
>  }
>  
> +JSString *
> +JSStructuredCloneReader::readString(uint32_t data)

I would've named this method something else, to distinguish it from the template, but I defer to your judgment.

	Ed Morley [:emorley]
	Comment 45 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/c88b1d63a5f8
https://hg.mozilla.org/mozilla-central/rev/c96053f6a716
https://hg.mozilla.org/mozilla-central/rev/167b98c4bc86

	Jan de Mooij [:jandem] Assignee
	Comment 46 • 10 years ago

Part 15:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0cbd074dcda

(In reply to Jason Orendorff [:jorendorff] from comment #44)
> I would've named this method something else, to distinguish it from the
> template, but I defer to your judgment.

I renamed the template one to readStringImpl.

	Carsten Book [:Tomcat]
	Comment 47 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/b0cbd074dcda