Closed
Bug 600173
Opened 14 years ago
Closed 14 years ago
atoms should live only in the default compartment
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
DUPLICATE
of bug 600593
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: igor, Assigned: gal)
References
Details
Attachments
(1 file, 8 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Reporter | ||
Comment 1•14 years ago
|
||
Nominating for 2.0 as this is a regression from bug 558861
blocking2.0: --- → ?
Reporter | ||
Comment 2•14 years ago
|
||
The patch copies the string if it does not come from the default compartment.
Assignee: general → igor
Attachment #479064 -
Flags: review?(anygregor)
Reporter | ||
Comment 3•14 years ago
|
||
In v1 I forgot to replace in flatLength() and flatLength() by just chars() and length() when js_AtomizeString copies the strings as now strings of any kind from a non-default-compartment are treated as temporary.
Attachment #479064 -
Attachment is obsolete: true
Attachment #479067 -
Flags: review?(anygregor)
Attachment #479064 -
Flags: review?(anygregor)
Reporter | ||
Comment 4•14 years ago
|
||
fixing comments
Attachment #479067 -
Attachment is obsolete: true
Attachment #479069 -
Flags: review?(anygregor)
Attachment #479067 -
Flags: review?(anygregor)
Comment 5•14 years ago
|
||
Comment on attachment 479069 [details] [diff] [review]
v3
>+ if (needNewString) {
> SwitchToCompartment sc(cx, cx->runtime->defaultCompartment);
>-
>+ AutoUnlockDefaultCompartment unlock();
>+ jschar *chars = str->chars();
> if (flags & ATOM_NOCOPY) {
>- key = js_NewString(cx, str->flatChars(), str->flatLength());
>+ key = js_NewString(cx, chars, length);
> if (!key)
> return NULL;
This means 2 threads could allocate a String in the same compartment at the same time.
That's not good!
Reporter | ||
Comment 6•14 years ago
|
||
(In reply to comment #5)
> This means 2 threads could allocate a String in the same compartment at the
> same time.
It is OK as only one thread wins the race and the key would be looked up again - see relookupOrAdd call.
Comment 7•14 years ago
|
||
(In reply to comment #6)
> (In reply to comment #5)
> > This means 2 threads could allocate a String in the same compartment at the
> > same time.
>
> It is OK as only one thread wins the race and the key would be looked up again
> - see relookupOrAdd call.
No there could be a race condition in the allocation code if 2 threads try to get an object from the freelist at the same time. The freelist is now in the compartment and not in ThreadData.
Assignee | ||
Comment 9•14 years ago
|
||
Why does this patch contain a bunch of unrelated changes to the int string code?
Comment 10•14 years ago
|
||
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > This means 2 threads could allocate a String in the same compartment at the
> > > same time.
> >
> > It is OK as only one thread wins the race and the key would be looked up again
> > - see relookupOrAdd call.
>
> No there could be a race condition in the allocation code if 2 threads try to
> get an object from the freelist at the same time. The freelist is now in the
> compartment and not in ThreadData.
Compartments are not single-threaded data structures?!
/be
Comment 11•14 years ago
|
||
(In reply to comment #10)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > This means 2 threads could allocate a String in the same compartment at the
> > > > same time.
> > >
> > > It is OK as only one thread wins the race and the key would be looked up again
> > > - see relookupOrAdd call.
> >
> > No there could be a race condition in the allocation code if 2 threads try to
> > get an object from the freelist at the same time. The freelist is now in the
> > compartment and not in ThreadData.
>
> Compartments are not single-threaded data structures?!
>
> /be
Yes they are but in the case of allocating atoms we have to change to the defaultCompartment and that's the only place where we have to make sure that 2 threads can't enter the allocation path at the same time.
Assignee | ||
Comment 12•14 years ago
|
||
Minimal patch. Just make a copy of the string if its in the wrong compartment.
NB: all compartments are single-threaded, _except_ the default compartment.
Assignee: igor → gal
Attachment #479069 -
Attachment is obsolete: true
Attachment #479069 -
Flags: review?(anygregor)
Assignee | ||
Updated•14 years ago
|
Attachment #479213 -
Flags: review?(anygregor)
Assignee | ||
Comment 13•14 years ago
|
||
Attachment #479213 -
Attachment is obsolete: true
Attachment #479213 -
Flags: review?(anygregor)
Updated•14 years ago
|
Attachment #479217 -
Flags: review+
Comment 16•14 years ago
|
||
Uh that doesn't work since we get a JSString from js_Atomize that's not heap allocated.
http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unittest&logfile=1285721694.1285721798.30677.gz&buildtime=1285721694&buildname=Rev3%20Fedora%2012%20tryserver%20debug%20test%20jsreftest&fulltext=1#err3
Assignee | ||
Comment 17•14 years ago
|
||
This should work
Attachment #479226 -
Attachment is obsolete: true
Reporter | ||
Comment 18•14 years ago
|
||
Comment on attachment 479259 [details] [diff] [review]
patch
>diff --git a/js/src/jsatom.cpp b/js/src/jsatom.cpp
>--- a/js/src/jsatom.cpp
>+++ b/js/src/jsatom.cpp
>@@ -454,25 +454,28 @@ JSAtom *
> js_AtomizeString(JSContext *cx, JSString *str, uintN flags)
> {
> JS_ASSERT(!(flags & ~(ATOM_PINNED|ATOM_INTERNED|ATOM_TMPSTR|ATOM_NOCOPY)));
> JS_ASSERT_IF(flags & ATOM_NOCOPY, flags & ATOM_TMPSTR);
>
> if (str->isAtomized())
> return STRING_TO_ATOM(str);
>
>+ str->ensureNotRope();
>+
> size_t length = str->length();
>+ jschar *chars = str->chars();
>+
> if (length == 1) {
>- jschar c = str->chars()[0];
>+ jschar c = chars[0];
> if (c < UNIT_STRING_LIMIT)
> return STRING_TO_ATOM(JSString::unitString(c));
> }
>
> if (length == 2) {
>- jschar *chars = str->chars();
> if (JSString::fitsInSmallChar(chars[0]) &&
> JSString::fitsInSmallChar(chars[1])) {
> return STRING_TO_ATOM(JSString::length2String(chars[0], chars[1]));
> }
> }
>
> /*
> * Here we know that JSString::intStringTable covers only 256 (or at least
>@@ -491,16 +494,34 @@ js_AtomizeString(JSContext *cx, JSString
> (chars[1] - '0') * 10 +
> (chars[2] - '0');
>
> if (jsuint(i) < INT_STRING_LIMIT)
> return STRING_TO_ATOM(JSString::intString(i));
> }
> }
>
>+ JS_ASSERT(!JSString::isStatic(str));
>+
>+ // If the string is not already in the default compartment, copy it there.
>+ if ((flags & ATOM_TMPSTR) ||
>+ (str->asCell()->compartment() != cx->runtime->defaultCompartment)) {
>+ SwitchToCompartment sc(cx, cx->runtime->defaultCompartment);
>+ AutoLockDefaultCompartment lock(cx);
>+
>+ // We conditionally unlock the lock if the default compartment is locked and we have
>+ // to GC.
>+ JSString *str2 = js_NewStringCopyN(cx, chars, length);
This will waste memory as the code ignores NOCOPY flag.
Reporter | ||
Comment 19•14 years ago
|
||
This patch removes unrelated changes from js_AtomizeString.
Attachment #479259 -
Attachment is obsolete: true
Attachment #479379 -
Flags: review?(anygregor)
Reporter | ||
Comment 20•14 years ago
|
||
(In reply to comment #9)
> Why does this patch contain a bunch of unrelated changes to the int string
> code?
That was from another patch where I initially found and fixed the issue.
Reporter | ||
Comment 21•14 years ago
|
||
(In reply to comment #11)
> Yes they are but in the case of allocating atoms we have to change to the
> defaultCompartment and that's the only place where we have to make sure that 2
> threads can't enter the allocation path at the same time.
Perhaps we should add separated atoms compartment? Or the goal is to ensure that only atoms would live in the default compartment effectively turning the default compartment into the default one.
Assignee | ||
Comment 22•14 years ago
|
||
Comment on attachment 479379 [details] [diff] [review]
v5
This patch (again) introduces a race condition by entering the allocation path unlocked. v5 also seems to have lost the ropes fix. v4 was tested and works. I need this bug fixed for the compartments landing. I will spin off a bug that doesn't block b7/final to further optimize this issue w.r.t. to memory use. For now I need this to be correct for the compartment landing.
Assignee | ||
Updated•14 years ago
|
Attachment #479259 -
Attachment is obsolete: false
Assignee | ||
Comment 23•14 years ago
|
||
Comment on attachment 479379 [details] [diff] [review]
v5
Moving v5 to a new bug.
Attachment #479379 -
Attachment is obsolete: true
Attachment #479379 -
Flags: review?(anygregor)
Reporter | ||
Comment 24•14 years ago
|
||
(In reply to comment #22)
> This patch (again) introduces a race condition by entering the allocation path
> unlocked.
I have missed that any allocations from the default compartment should be under the compartment lock. But that means that OOM reporting will happen under that lock as well. I guess js_ReportOutOfMemory should release that lock when executing the various user-supplied callbacks. OK, that should go to another bug.
> v5 also seems to have lost the ropes fix.
There is no need for it as str->chars() ensures that the string is not a rope, see comments in the code.
> v4 was tested and works.
That patch duplicates the string even if it is already in the table.
Assignee | ||
Comment 25•14 years ago
|
||
Attachment #479259 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #479440 -
Flags: review?(anygregor)
Comment 26•14 years ago
|
||
If the default compartment is for atoms only, can we unify locking for allocation with existing atom state locking? Too many locks...
Also, holding locks across error reports is bad form. Elsewhere we release first, and this sometimes makes for a protocol where a callee is invoked with a lock held and if it succeeds, the lock is still held and invariants are maintained, but if it fails, the lock was released and an error has been reported. This is a valid pattern, we should use it if we can.
Abstracting over held-locks across hard-to-delimit critical sections and failure paths is almost always a design mistake.
/be
Assignee | ||
Comment 27•14 years ago
|
||
(In reply to comment #26)
> If the default compartment is for atoms only, can we unify locking for
> allocation with existing atom state locking? Too many locks...
We already do that. Thats the lock we hold.
>
> Also, holding locks across error reports is bad form. Elsewhere we release
We don't do that. We release the lock before reporting.
Reporter | ||
Comment 28•14 years ago
|
||
(In reply to comment #27)
> We don't do that. We release the lock before reporting.
The OOM is still reported with the compartment lock held.
Reporter | ||
Updated•14 years ago
|
Version: 1.9.2 Branch → Trunk
Assignee | ||
Comment 29•14 years ago
|
||
Which one? The one in Atomize? We can add an AutoUnlock in that branch. Gregor want to do that?
Reporter | ||
Comment 30•14 years ago
|
||
(In reply to comment #29)
> Which one? The one in Atomize?
The one in RefillFinalizableFreeList, in str->undepend() and during allocation of the char array in js_NewStringCopyN.
Reporter | ||
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → DUPLICATE
Updated•14 years ago
|
blocking2.0: ? → betaN+
Updated•13 years ago
|
Attachment #479440 -
Flags: review?(anygregor)
You need to log in
before you can comment on or make changes to this bug.
Description
•