If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Compiler flag for JS_CStringsAreUTF8

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: gwagner, Assigned: gwagner)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
Andreas wanted it.
(Assignee)

Updated

8 years ago
Assignee: general → anygregor
Don't break CouchDB.

/be
(Assignee)

Comment 2

8 years ago
I downloaded the couchDB source version 0.10.1 but I couldn't find any use of JS_CStringsAreUTF8 or JS_SetCStringsAreUTF8.
It starts like: 

int
main(int argc, const char * argv[]) {
    JSRuntime *runtime;
    runtime = JS_NewRuntime(64L * 1024L * 1024L);

Am I missing something here?
(Assignee)

Comment 3

8 years ago
Created attachment 434679 [details] [diff] [review]
WiP

First version.

Do we still want to enable it with JS_SetCStringsAreUTF8(void) or should we enable it by default once the compiler flag is set?
(Assignee)

Comment 4

8 years ago
The flusspferd library uses it. http://flusspferd.org/.
(Assignee)

Comment 5

8 years ago
Created attachment 437075 [details] [diff] [review]
refresh
Attachment #434679 - Attachment is obsolete: true
(Assignee)

Comment 6

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

Any comments on how to enable CStringsAreUtf8? Compile flag and enable it manually or enable by default if compile flag is set?
Attachment #437075 - Attachment is obsolete: true
Attachment #437086 - Flags: review?(gal)

Comment 7

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

I am not too familiar with the build system magic.
Attachment #437086 - Flags: review?(gal) → review?(jim)

Comment 8

8 years ago
GPSEE uses this flag too, as does my bit-rotted patch for getting UTF8 cleanly into JS_CompileFile().

IMHO, this patch is a fail: for me to produce a single embedding which can do either UTF8 or plain C strings, I would need to link against *two* different libmozjs.so's. That's completely ridiculous.

If you want take JS_CStringsAreUTF8 out of the browser code, fine - but please leave JSAPI alone for the embedders.
(Assignee)

Comment 9

8 years ago
> 
> IMHO, this patch is a fail: for me to produce a single embedding which can do
> either UTF8 or plain C strings, I would need to link against *two* different
> libmozjs.so's. That's completely ridiculous.
> 
> If you want take JS_CStringsAreUTF8 out of the browser code, fine - but please
> leave JSAPI alone for the embedders.

Well that's why I was asking how it should be enabled and I didn't get an answer last time.

Comment 10

8 years ago
I can't really tell you *how* it should be enabled: I don't know what problem  this is trying to solve.  IIRC, this was the state of JSAPI in 2007, do you know why the JS_CStringsAreUTF8 API was added in the first place?

I *can* tell you that permanently removing features from API consumers is likely to cause some of us problems, and is generally contrary to the way Mozilla "does business" with this product (JSAPI).

Oh -- re. "I didn't get an answer last time" -- I can't find your message on the dev-tech-js-engine mailing list. When did you post it?

Comment 11

8 years ago
Gal says he wants to 
 - to eliminate branchiness
 - not elimiante UTF-8 C Strings
 - (presumably) not elimiante non-UTF-8 C Strings

Problem: some embeddings can use both (on different JS programs); that means either branchiness or linking against two different libmozjs.so (which could get very ugly indeed)

Out of curiosity, has anybody ever benchmarked the penalty that checking js_StringsAreUTF8() imparts?
Still a valid non-TM bug?
(Assignee)

Comment 13

6 years ago
(In reply to Ryan VanderMeulen from comment #12)
> Still a valid non-TM bug?

Yes.
Summary: TM: Compiler flag for JS_CStringsAreUTF8 → Compiler flag for JS_CStringsAreUTF8

Comment 14

6 years ago
Comment on attachment 437086 [details] [diff] [review]
patch

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

It sounds like Wes and Gregor don't agree on whether this patch is the right way to achieve the goals that Andreas set out.

One way to eliminate branchiness for Mozilla but to allow embedders to switch would be to keep the compile flag, but have it determine whether js_CStringsAreUTF8 is an initialized const (for Mozilla, to false, it seems?) or a variable (for other embeddings). The compile flag would drop the definition of JS_SetCStringsAreUTF8 altogether in the non-Mozilla case.

I haven't checked the code, but I'm pretty sure the compiler will just treat initialized const variables as compile-time constants, and drop the branches.
Attachment #437086 - Flags: review?(jimb)

Comment 15

6 years ago
"keep the configure flag", I meant.
I removed CStringsAreUTF8 in Bug 805080 changeset 111811:b2620ac72e94.  We have had separate API entry points for ASCII, UTF-8, and 2-byte-chars for almost a year now.  API users can just use what they need without the awkward modality.

In the not-too-distant-future, we will be linking against the ICU library to support the Intl module, so that should reduce the burden of supporting different encodings even more.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.