Closed Bug 1211723 Opened 9 years ago Closed 8 years ago

share ScriptSource between JSRuntimes via parentRuntime

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: luke, Assigned: fitzgen)

References

Details

(Whiteboard: [MemShrink:P1])

Attachments

(9 files, 35 obsolete files)

441.42 KB, image/png
Details
130.59 KB, image/png
Details
66.52 KB, image/png
Details
417.97 KB, application/x-gzip
Details
427.63 KB, application/x-gzip
Details
65.00 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
30.90 KB, patch
jimb
: review+
Details | Diff | Splinter Review
2.67 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.65 KB, patch
fitzgen
: feedback+
Details | Diff | Splinter Review
With multiple workers (esp. running threaded SharedArrayBuffer code), it's likely that scripts will be duplicated between workers.  With asm.js code, the scripts can be quite large (80mb).  There is already a JSRuntime::compressedSourceSet that shares ScriptSource within a JSRuntime, so the logical extension would be to share this field via JSRuntime::parentRuntime, protected by a lock and for the ScriptSource refct to be atomic.

On additional requirement for this to really help asm.js is that the compressedSourceSet be generalized to include permanently-uncompressed sources (due to performance faults involving compression of huge scripts, scripts bigger than 5MiB [1] are left permanently decompressed).  I think this is pretty simple.

[1] http://hg.mozilla.org/mozilla-central/file/67adec79eb8a/js/src/jsscript.cpp#l1919
See Also: → 1211006
Bug 1244841 comment 7 is a great example of this: 64 MB of code duplicated across the main thread and 16 workers!
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P1]
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
This patch series is enough to get the jit-tests passing, but there are a few bugs.

The filename_, displayURL_, and sourceMapURL_ members are all given out such that references exist longer than the SharedScriptSource data is exclusively locked, for example via JSScript::filename(). Meaning we have some data races that could lead to use-after-frees. I think we could do some kind of immutable-after-initialization thing for filename_ and displayURL_. However, we actually have setters for the sourceMapURL_ field exposed to chrome JS via Debugger.Source.prototype.sourceMapURL, so the immutable-after-init approach wouldn't work here.

I think I took a slightly wrong approach by making all of ScriptSource and its filename/etc shared between runtimes. Instead, I think we should still have each ScriptSource belong to a runtime, and share only the source text content between runtimes. This would narrow the scope of the sharing to something more manageable and still give us the vast majority of benefits as the filename/etc are tiny compared to the source text contents.
Attachment #8726503 - Attachment is obsolete: true
Attachment #8726504 - Attachment is obsolete: true
Attachment #8726505 - Attachment is obsolete: true
Attachment #8726506 - Attachment is obsolete: true
OK, so this time around we are sharing only the underlying source text and not
any of the source's metadata, such as the filename.

Passing most jit-tests, but there is one worrying failure related to
compression/decompression and zlib, and a bunch of OOM-test failures related to
the way I hacked up an infallible ExclusiveData constructor while biding time
for when it really becomes infallible to create.

Going to pause work here for a couple days as I help Till out with
Debugger-related issues for Promises and take a look at some other crashers.

I'm going to eventually flag both of you (:jimb, :terrence) for review, because
this is a little bit sticky and I want to make sure we do it right ;)
TODO:

* Fix failing tests (obviously)

* Add an `UncompressedSourceSet` to the parent JSRuntime, which is similar to the `CompressedSourceSet` but for (you guessed it) sources we aren't compressing for whatever reason.

* Add a `js::SharedSourceText::Create()` static method that checks the parent runtime's source sets to de-duplicate new sources, and use this where we are otherwise using js_new<SharedSourceText>() currently.
Depends on: 1257019
Attachment #8728017 - Attachment is obsolete: true
Attachment #8728018 - Attachment is obsolete: true
Attachment #8728019 - Attachment is obsolete: true
Attachment #8728020 - Attachment is obsolete: true
SharedSourceText is atomically refcounted and shared between JSRuntime
threads. The actual chars are stored in SharedSourceText::SourceText, which is
wrapped in an ExclusiveData to ensure that only one thread has access at a
time. A SourceText is either Compressed or Uncompressed.

This also adds an UncompressedSourceSet to the parent runtime. When creating new
sources, we check this set first and return an extant source if we already have
one for these chars. Compressing a source's text removes it from this set and
adds it to the parent runtime's CompressedSourceSet which also performs
deduplication.

Bug 1211723 - Part 4: Create a set for deduplicating uncompressed sources on the parent runtime; r=terrence,jimb
Attachment #8731888 - Flags: review?(terrence)
Attachment #8731888 - Flags: review?(jimb)
This patch series shaves almost a full GB of memory usage off from the testcase in bug 1244841 comment 0.
Try push (there are likely still a few issues that haven't been uncovered just from jit tests locally yet):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e2920807a00
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #17)
> This patch series shaves almost a full GB of memory usage off from the
> testcase in bug 1244841 comment 0.

Very nice!
Comment on attachment 8731885 [details] [diff] [review]
Part 0: Add asserts against re-entrancy to js::ExclusiveData<T>

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

::: js/src/threading/ExclusiveData.h
@@ +95,5 @@
>      ExclusiveData(const ExclusiveData&) = delete;
>      ExclusiveData& operator=(const ExclusiveData&) = delete;
>  
> +    void acquire() const {
> +        lock_.lock();

Maybe also assert !lockOwner_ here?
Attachment #8731885 - Flags: review?(terrence) → review+
Comment on attachment 8731886 [details] [diff] [review]
Part 1: Share CompressedSourceSet between JSRuntimes on the parentRuntime

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

::: js/src/vm/Runtime.cpp
@@ +547,5 @@
>  
>      rtSizes->uncompressedSourceCache += uncompressedSourceCache.sizeOfExcludingThis(mallocSizeOf);
>  
> +    {
> +        auto set = compressedSourceSet().lock();

This way, the parent and children all count the source set as their own. Wouldn't it make more sense to attribute the source set to the parent alone?
Attachment #8731886 - Flags: review?(jimb) → review+
Comment on attachment 8731887 [details] [diff] [review]
Part 2: Share UncompressedSourceCache between JSRuntimes on the parentRuntime

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

::: js/src/vm/Runtime.cpp
@@ +553,5 @@
>  
> +    {
> +        auto cache = uncompressedSourceCache().lock();
> +        rtSizes->uncompressedSourceCache += cache->sizeOfExcludingThis(mallocSizeOf);
> +    }

Similarly to the other patch: perhaps we should attribute this only to the parent.
Attachment #8731887 - Flags: review?(jimb) → review+
Comment on attachment 8731888 [details] [diff] [review]
Part 3: Create SharedSourceText to share ScriptSource's underlying chars between runtimes

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

The comment on the patch ought to be in the source somewhere, and in much more detail. The comments in jsscript.h are inadequate; there needs to be a general exposition on where the shared data lives, how it moves in and out of the various caches, who can point to it when, and so on; it's not okay to make people infer these things by following how the members are used.

When there's enough of an explanation that I can orient myself and check the patch against the explanation, then I'll re-review. It had better be easy for me, because once it's landed, other people encountering this code will not be as interested in it as I am, as the reviewer.

::: js/src/jsscript.h
@@ +559,5 @@
> +struct UncompressedSourceLookup;
> +
> +/**
> + * The `SharedSourceText` class enables sharing source text contents between
> + * JSRuntime threads. `SharedSourceText` is atomically reference counted, and

"between JSRuntime threads" makes me think of threads belonging to a particular JSRuntime. What I think you mean is "enables sharing source text contents between multiple threads using multiple JSRuntimes", right?

@@ +582,5 @@
> +        UniquePtr<char16_t[], JS::FreePolicy> chars;
> +        // Length of the chars. Note that chars may contain null bytes.
> +        size_t length;
> +        // True iff this owner's SharedSourceText is in the parent runtime's
> +        // UncompressedSourceSet.

I don't really understand this comment. Which owner? Do you mean:

True if the owner of the SourceText to which this belongs is in the parent runtime's ...

@@ +601,5 @@
> +          : chars(mozilla::Move(rhs.chars))
> +          , length(rhs.length)
> +          , inUncompressedSourceSet(rhs.inUncompressedSourceSet)
> +          , hash(mozilla::Move(rhs.hash))
> +        { }

Should this have a self-move check?

@@ +618,5 @@
> +        HashNumber hash;
> +        // Length of the *un*compressed data.
> +        size_t length;
> +        // True iff this owner's SharedSourceText is in the parent runtime's
> +        // CompressedSourceSet.

Same comment as above.

@@ +648,5 @@
> +     * The source text is either compressed or uncompressed.
> +     */
> +    class SourceText : public mozilla::Variant<Compressed, Uncompressed>
> +    {
> +        friend class SharedSourceText;

This friend declaration isn't necessary, since we're a member class of SharedSourceText.

@@ +749,5 @@
> +    // incremented for the caller.
> +    //
> +    // `F intoOwnedChars` should return a `UniquePtr<char16_t[], JS::FreePolicy>`
> +    // version of chars. This allows callers to elide a copy of the chars when
> +    // possible. It should handle reporting exceptions on the context itself.

You should provide the signature for intoOwnedChars, and expand on this 'elide copy' thing.
Attachment #8731888 - Flags: review?(jimb) → review-
(In reply to Jim Blandy :jimb from comment #23)
> When there's enough of an explanation that I can orient myself and check the
> patch against the explanation, then I'll re-review. It had better be easy
> for me, because once it's landed, other people encountering this code will
> not be as interested in it as I am, as the reviewer.

My tone here was not great; I'm sorry about that. I was in a hurry to get home and didn't re-read. What I should have said was:

As the reviewer, I'm ready to invest energy in understanding how the patch works, so I can have confidence it's correct before I approve it. But subsequent readers will have some other purpose in mind when they encounter this code, so if it's hard for me to understand, it's *definitely* going to be hard for them.
Comment on attachment 8731886 [details] [diff] [review]
Part 1: Share CompressedSourceSet between JSRuntimes on the parentRuntime

These patches aren't atomic, like I thought they were. Canceling review for now, squashing the patches, fixing failures in that try push, and then I will reflag.
Attachment #8731886 - Flags: review?(terrence)
Comment on attachment 8731887 [details] [diff] [review]
Part 2: Share UncompressedSourceCache between JSRuntimes on the parentRuntime

Ditto.
Attachment #8731887 - Flags: review?(terrence)
Comment on attachment 8731888 [details] [diff] [review]
Part 3: Create SharedSourceText to share ScriptSource's underlying chars between runtimes

You know the deal
Attachment #8731888 - Flags: review?(terrence)
Ok, Jim and I just talked and discovered a potential deadlock condition based on the order we lock the sets and individual source texts in different places. Need some more refactoring.
Attachment #8731885 - Attachment is obsolete: true
Attachment #8731886 - Attachment is obsolete: true
Attachment #8731887 - Attachment is obsolete: true
Attachment #8731888 - Attachment is obsolete: true
Lots of bug fixes! Assertions against dead locks! Woo!

Let's see if this try push does any better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca393f95c122
Add missing header includes, add missing namespace qualifiers, and fix bad
include orders.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f709fca7517
Attachment #8733135 - Attachment is obsolete: true
Sigh... I forgot one header include. I am clearly getting a different unified
grouping than the try builds...

Yet Another Try Push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=025fc6d02469
Attachment #8733145 - Attachment is obsolete: true
Ok, I finally set FILES_PER_UNIFIED_FILE=1 locally and worked out all the
kinks. Should have done that way sooner...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e280df892733
Attachment #8733148 - Attachment is obsolete: true
Ok, this is getting much closer. Jit tests passing, most mochitests and xpcshell tests passing! A few assertion failures, but they all seem to be pretty concrete and point right to where the bugs are.

A lot of build failures in my attempt to stub out nspr threads for DEBUG only assertions. I think I should just include jslock.h or whatever js thing interfaces with nspr until we have js::Thread::current().
Comment on attachment 8733134 [details] [diff] [review]
Part 0: Add asserts against re-entrancy to js::ExclusiveData<T>

This is done more better-er and more general-er in bug 1258847.
Attachment #8733134 - Attachment is obsolete: true
The failing mochitests on OSX are pthread_mutex_lock returning EINVAL which is either:

* "The mutex was created with the protocol attribute having the value PTHREAD_PRIO_PROTECT
   and the calling thread's priority is higher than the mutex's current priority ceiling."

* "The value specified by mutex does not refer to an initialized mutex object."

We don't use PTHREAD_PRIO_PROTECT in js::Mutex, so I suspect it must be the latter. Still debugging.

Haven't looked into the windows failures yet.
Ok, need to back up and simplify the design a bit. As it is now, this is trying to be too fancy, too fine grained w/ locking, and it makes destroying shared sources a huge pain because of inversion issues, and GC interaction, and the safety of the refcounting, etc. I keep fixing Just One More Thing and the architecture gets more and more convoluted. Even if we got it into a place where it was working correctly now, we wouldn't be able to solve any bugs introduced in the future.

Going to back up and have a single lock protect a cache of shared and immutable strings. It will be totally orthogonal to ScriptSource and various source text related things. Then, after this dead simple cache of shared and immutable strings is implemented and solid, I will start making ScriptSource's text contents use it. The strings will be opaque to the cache, it will have no idea what they are for, if the data is compressed or not, etc. I think this design will be much simpler to implement and debug.
For posterity, here is my current patch that tries to be way too clever and fine
grained with the locking.
Attachment #8733169 - Attachment is obsolete: true
Attachment #8734195 - Attachment is obsolete: true
This adds a SharedImmutableStringsCache to every JSRuntime, but the accessor
always returns the parent runtime's cache. The caches in child JSRuntime's are
not wasting space, however, as initialization and allocation of the table
happens lazily within SharedImmutableStringsCache.
Attachment #8734826 - Flags: review?(jimb)
This commit removes `js::ScriptSource::Parent` and the `CompressedSourceSet`.
They are unnecessary because source text is always shared via the parent
runtime's `SharedImmutableStringsCache` now.
Attachment #8734829 - Flags: review?(jimb)
This approach is *SO* much easier than the last one!

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8d10c9be94b
Comment on attachment 8734827 [details] [diff] [review]
Part 2: Move source length from ScriptSource into the data variants

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

::: js/src/jsscript.h
@@ +638,5 @@
>  
>          void* raw;
>          size_t nbytes;
>          HashNumber hash;
> +        size_t length;

Note that this patch is mostly busy work, but makes the following parts a little bit more lucid.
Attachment #8734825 - Attachment is obsolete: true
Attachment #8734825 - Flags: review?(jimb)
Attachment #8734828 - Attachment is obsolete: true
Attachment #8734828 - Flags: review?(jimb)
Fixing up some warnings-as-errors fallout.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c097296f2b2
Fix dumb bug introduced when fixing warnings-as-errors bugs.
Attachment #8734933 - Flags: review?(jimb)
Attachment #8734885 - Attachment is obsolete: true
Attachment #8734885 - Flags: review?(jimb)
Pro-tip: about:memory is just text. You can copy and paste its contents :)
Heh, good call :)
Attachment #8734826 - Flags: review?(jimb) → review+
Comment on attachment 8734933 [details] [diff] [review]
Part 0: Create a cache for shared, immutable strings

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

Really great stuff.

::: js/src/jsapi-tests/testSharedImmutableStringsCache.cpp
@@ +65,5 @@
> +    CHECK(threads.reserve(NUM_THREADS));
> +
> +    for (auto i : mozilla::MakeRange(NUM_THREADS)) {
> +        auto cacheAndIndex = js_new<CacheAndIndex>(&cache, i);
> +        CHECK(cacheAndIndex);

optional nit: You could probably add a PRThread* member to CacheAndIndex (I'd call it ThreadClosure) and then make `threads` a vector of those.

::: js/src/vm/SharedImmutableStringsCache.cpp
@@ +35,5 @@
> +{
> +    auto clone = cache_->getOrCreate(chars(), length(), [&]() {
> +        MOZ_CRASH("Should not need to create an owned version, as this string is already in "
> +                  "the cache");
> +        return nullptr;

Isn't there an always-crash "unreachable" macro?

::: js/src/vm/SharedImmutableStringsCache.h
@@ +10,5 @@
> +#include "mozilla/Maybe.h"
> +#include "mozilla/UniquePtr.h"
> +
> +#include <cstring>
> +#include <new>

Could we put '// for placement new' here? (Vector.h does this, and I would have found it helpful.)

@@ +34,5 @@
> +{
> +    friend class SharedImmutableStringsCache;
> +    friend class SharedImmutableTwoByteString;
> +
> +    SharedImmutableStringsCache* cache_;

It would be helpful to comment, "Never nullptr in a live instance. May be nullptr if this instance has been moved from."

@@ +38,5 @@
> +    SharedImmutableStringsCache* cache_;
> +    const char* chars_;
> +    size_t length_;
> +#ifdef DEBUG
> +    HashNumber hash_;

Just checking: DebugOnly doesn't serve here because it's a field, not a local, so it can't be entirely eliminated, and instead occupies a byte even in non-DEBUG builds. *sigh*

@@ +49,5 @@
> +#ifdef DEBUG
> +      , hash_(mozilla::HashString(chars, length))
> +#endif
> +    {
> +        MOZ_ASSERT(chars);

Assert `cache` too?

@@ +112,5 @@
> +class SharedImmutableTwoByteString
> +{
> +    friend class SharedImmutableStringsCache;
> +
> +    SharedImmutableString string_;

So the idea is, if a char and char16_t string just happen to be the same bytes, we'll just hand it out under two types? That's fine, but it would be worth spelling out, in a comment on this member.

@@ +118,5 @@
> +    explicit SharedImmutableTwoByteString(SharedImmutableString&& string)
> +      : string_(mozilla::Move(string))
> +    { }
> +
> +    SharedImmutableTwoByteString(SharedImmutableStringsCache* cache, const char* chars, size_t length)

Perhaps I'll take this back when I see how it's used, but I was expecting this to take char16_t, and then cast it when initializing `string_`.

@@ +119,5 @@
> +      : string_(mozilla::Move(string))
> +    { }
> +
> +    SharedImmutableTwoByteString(SharedImmutableStringsCache* cache, const char* chars, size_t length)
> +      : string_(cache, chars, length)

Assert that `length` is a multiple of sizeof(char16_t)?

@@ +177,5 @@
> +
> +    /**
> +     * Get the canonical, shared, and de-duplicated version of the given `const
> +     * char*` string. In the case that such a string does not exist,
> +     * `intoOwnedChars` is called and the string is added to the cache.

So great.

No need for passive voice. "If such a string does not exist, call `intoOwnedChars` and add the string it returns to the cache."

@@ +207,5 @@
> +            OwnedChars ownedChars(intoOwnedChars());
> +            if (!ownedChars)
> +                return mozilla::Nothing();
> +            MOZ_ASSERT(ownedChars.get() == chars ||
> +                       memcmp(ownedChars.get(), chars, length) == 0);

delightful

@@ +273,5 @@
> +            OwnedTwoByteChars ownedTwoByteChars(intoOwnedTwoByteChars());
> +            if (!ownedTwoByteChars)
> +                return mozilla::Nothing();
> +            MOZ_ASSERT(ownedTwoByteChars.get() == chars ||
> +                       memcmp(ownedTwoByteChars.get(), chars, length) == 0);

length * sizeof(char16_t), right?

@@ +297,5 @@
> +    MOZ_WARN_UNUSED_RESULT mozilla::Maybe<SharedImmutableTwoByteString>
> +    getOrCreate(OwnedTwoByteChars&& chars, size_t length);
> +
> +    /**
> +     * Do not take ownership of the given `chars`, return the canonical, shared

"`chars`;" instead of "`chars`," or full stop?

@@ +299,5 @@
> +
> +    /**
> +     * Do not take ownership of the given `chars`, return the canonical, shared
> +     * and de-duplicated version. If there is no extant shared version of
> +     * `chars`, then a copy will made.

"will be made"

@@ +394,5 @@
> +            return memcmp(key->chars(), lookup.chars_, key->length()) == 0;
> +        }
> +    };
> +
> +    using Set = HashSet<StringBox::Ptr, Hasher, SystemAllocPolicy>;

Could HashSet hold StringBoxes directly? StringBox holds a pointer to the chars, so they'll always be stable. SharedImmutableString re-hashes rather than pointing to a StringBox, so it doesn't need the StringBox to be stable.
Attachment #8734933 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #58)
> Comment on attachment 8734933 [details] [diff] [review]
> Part 0: Create a cache for shared, immutable strings
> 
> Review of attachment 8734933 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Really great stuff.

Thanks!

> @@ +38,5 @@
> > +    SharedImmutableStringsCache* cache_;
> > +    const char* chars_;
> > +    size_t length_;
> > +#ifdef DEBUG
> > +    HashNumber hash_;
> 
> Just checking: DebugOnly doesn't serve here because it's a field, not a
> local, so it can't be entirely eliminated, and instead occupies a byte even
> in non-DEBUG builds. *sigh*

Yes, and also DebugOnly is a MOZ_STACK_CLASS these days.

> @@ +394,5 @@
> > +            return memcmp(key->chars(), lookup.chars_, key->length()) == 0;
> > +        }
> > +    };
> > +
> > +    using Set = HashSet<StringBox::Ptr, Hasher, SystemAllocPolicy>;
> 
> Could HashSet hold StringBoxes directly? StringBox holds a pointer to the
> chars, so they'll always be stable. SharedImmutableString re-hashes rather
> than pointing to a StringBox, so it doesn't need the StringBox to be stable.

Good catch!
Attachment #8734827 - Flags: review?(jimb) → review+
Comment on attachment 8734886 [details] [diff] [review]
Part 3: Make ScriptSource use the SharedImmutableStringsCache for its source text

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

::: js/src/jsscript.cpp
@@ +1797,5 @@
> +    mozilla::UniquePtr<char16_t[], JS::FreePolicy> ownedSource(src);
> +    auto& cache = cx->runtime()->sharedImmutableStrings();
> +    auto deduped = cache.getOrCreate(mozilla::Move(ownedSource), length);
> +    if (!deduped) {
> +        ReportOutOfMemory(cx);

We're introducing this boilerplate in several places, as we lift up the de-duplication to the higher level. It's never nice to see call sites being made more complicated. Would it be possible to give ScriptSource its own methods that take care of this?

(Perhaps there's a later patch in the queue...)

::: js/src/jsstr.cpp
@@ +4754,5 @@
> +    if (!ret)
> +        return nullptr;
> +    PodCopy(ret.get(), s, n);
> +    ret[n] = 0;
> +    return ret;

How is this legal? Isn't this an attempt to copy-assign a UniqueChars?
Attachment #8734886 - Flags: review?(jimb) → review+
Comment on attachment 8734829 [details] [diff] [review]
Part 4: Remove source de-duplication efforts that have been superseded

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

This is my favorite patch so far.

::: js/src/jsscript.h
@@ +701,3 @@
>      {
>      }
> +    ~ScriptSource() { }

Isn't this the same as just omitting any explicit destructor?

::: js/src/vm/Runtime.h
@@ -1286,5 @@
>      js::UncompressedSourceCache uncompressedSourceCache;
>      js::EvalCache       evalCache;
>      js::LazyScriptCache lazyScriptCache;
>  
> -    js::CompressedSourceSet compressedSourceSet;

YESSSS
Attachment #8734829 - Flags: review?(jimb) → review+
Comment on attachment 8734886 [details] [diff] [review]
Part 3: Make ScriptSource use the SharedImmutableStringsCache for its source text

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

::: js/src/jsscript.cpp
@@ +2042,2 @@
>  
> +    data = SourceType(Compressed(mozilla::Move(raw), hash, length));

I really like how this automatically takes care of dropping a reference from the compressed text.
Attachment #8734830 - Flags: review?(jimb) → review+
Attachment #8734831 - Flags: review?(jimb) → review+
Really nice. Makes it look easy. LOL
As a sanity check it would be great if you could do a DMD run prior to landing [1]. This will help identify any significantly large unreported chunks we may have missed as well as possible double-reporting.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/DMD
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(nfitzgerald)
Eric, I'm not 100% sure what I'm looking for in these dmd output files. I ran dmd.py on the output with the patch series applied. I don't have any double counting, and I'm not finding any unreported chunks with `SharedImmutableStringsCache`-related things on the allocation stack. Anything else I should look for?
Flags: needinfo?(erahm)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #67)
> Eric, I'm not 100% sure what I'm looking for in these dmd output files. I
> ran dmd.py on the output with the patch series applied. I don't have any
> double counting, and I'm not finding any unreported chunks with
> `SharedImmutableStringsCache`-related things on the allocation stack.
> Anything else I should look for?

Yeah, high level it looks good. Thanks for taking time to measure.
Flags: needinfo?(erahm)
(In reply to Jim Blandy :jimb from comment #58)
> @@ +35,5 @@
> > +{
> > +    auto clone = cache_->getOrCreate(chars(), length(), [&]() {
> > +        MOZ_CRASH("Should not need to create an owned version, as this string is already in "
> > +                  "the cache");
> > +        return nullptr;
> 
> Isn't there an always-crash "unreachable" macro?

Unfortunately, there's only either 

* MOZ_ASSERT_UNREACHABLE: debug only assert, and still requires "default" return for release builds.

* MOZ_MAKE_COMPILER_ASSUME_UNREACHABLE: makes the compiler consider the block unreachable and results in UB if it is reached.

But what I want is more like MOZ_ASSERT_UNREACHABLE that is not debug only. I suppose I can create MOZ_RELEASE_ASSERT_UNREACHABLE...
(In reply to Jim Blandy :jimb from comment #60)
> Comment on attachment 8734886 [details] [diff] [review]
> Part 3: Make ScriptSource use the SharedImmutableStringsCache for its source
> text
> 
> Review of attachment 8734886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsscript.cpp
> @@ +1797,5 @@
> > +    mozilla::UniquePtr<char16_t[], JS::FreePolicy> ownedSource(src);
> > +    auto& cache = cx->runtime()->sharedImmutableStrings();
> > +    auto deduped = cache.getOrCreate(mozilla::Move(ownedSource), length);
> > +    if (!deduped) {
> > +        ReportOutOfMemory(cx);
> 
> We're introducing this boilerplate in several places, as we lift up the
> de-duplication to the higher level. It's never nice to see call sites being
> made more complicated. Would it be possible to give ScriptSource its own
> methods that take care of this?
> 
> (Perhaps there's a later patch in the queue...)

Will do this as a follow up.

> 
> ::: js/src/jsstr.cpp
> @@ +4754,5 @@
> > +    if (!ret)
> > +        return nullptr;
> > +    PodCopy(ret.get(), s, n);
> > +    ret[n] = 0;
> > +    return ret;
> 
> How is this legal? Isn't this an attempt to copy-assign a UniqueChars?

Per spec, there are certain scenarios where copies/moves can be elided and most of them involve return values. This is one of them.
Addressed review comments and squashed the patches into a single commit.
Attachment #8736047 - Flags: review+
Attachment #8734826 - Attachment is obsolete: true
Attachment #8734827 - Attachment is obsolete: true
Attachment #8734829 - Attachment is obsolete: true
Attachment #8734830 - Attachment is obsolete: true
Attachment #8734831 - Attachment is obsolete: true
Attachment #8734886 - Attachment is obsolete: true
Attachment #8734933 - Attachment is obsolete: true
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/06a8c115f8fa for rather clear and honest assertion failures on at least Windows 7 xpcshell, https://treeherder.mozilla.org/logviewer.html#?job_id=24825601&repo=mozilla-inbound, and deeply disingenuous and probably lying crashes that are actually eaten assertions with only the crash vomited up from Mac and Windows debug Marionette, https://treeherder.mozilla.org/logviewer.html#?job_id=24824549&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=24829980&repo=mozilla-inbound
Ok all 3 failures are assertion failures that we don't create any dangling pointers when destroying the SharedImmutableStringsCache (ie, the refcount of each string in the cache is 0).

These are all under ~JSRuntime, meaning we have already done a shutdown GC. And there are still `ScriptSource`s hanging around after their JSRuntime has been destroyed. That's pretty surprising and scary in and of itself.

All that said, I haven't been able to reproduce locally yet.
Here is a try push without this patch series that puts all ScriptSources into a linked list on the runtime and asserts this list is empty upon destruction. If we are already leaking ScriptSources, it should show up in this push.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41855b016a72
Depends on: 1260891
I have a pretty good hunch what may be going on here in bug 1260891.
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #77)
> I have a pretty good hunch what may be going on here in bug 1260891.

This was incorrect (although it seems good to take that patch anyways).

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #76)
> Here is a try push without this patch series that puts all ScriptSources
> into a linked list on the runtime and asserts this list is empty upon
> destruction. If we are already leaking ScriptSources, it should show up in
> this push.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=41855b016a72

This was right. We are leaking `ScriptSource`s in Marionette tests, somehow:

>  18:02:54    ERROR -  PROCESS-CRASH | runner.py | application crashed [@ mozilla::LinkedList<js::ScriptSource>::~LinkedList()]
> 18:02:54     INFO -  Crash dump filename: /var/folders/n5/8vlsdw_12tddqc4b5pnh50t000000w/T/tmp4PXDcq.mozrunner/minidumps/64028645-A8CD-432A-A777-5D8333F3F330.dmp
> 18:02:54     INFO -  Operating system: Mac OS X
> 18:02:54     INFO -                    10.10.5 14F27
> 18:02:54     INFO -  CPU: amd64
> 18:02:54     INFO -       family 6 model 69 stepping 1
> 18:02:54     INFO -       4 CPUs
> 18:02:54     INFO -  Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
> 18:02:54     INFO -  Crash address: 0x0
> 18:02:54     INFO -  Process uptime: 8 seconds
> 18:02:54     INFO -  Thread 0 (crashed)
> 18:02:54     INFO -   0  XUL!mozilla::LinkedList<js::ScriptSource>::~LinkedList() [LinkedList.h:41855b016a72 : 329 + 0x0]
> 18:02:54     INFO -      rax = 0x0000000000000000   rdx = 0x00007fff794851f8
> 18:02:54     INFO -      rcx = 0x0000000000000000   rbx = 0x00007fff79485c50
> 18:02:54     INFO -      rsi = 0x00004e0000004e00   rdi = 0x00004d0000004e03
> 18:02:54     INFO -      rbp = 0x00007fff5fbfee20   rsp = 0x00007fff5fbfee10
> 18:02:54     INFO -       r8 = 0x00007fff5fbfedc0    r9 = 0x00007fff78a30300
> 18:02:54     INFO -      r10 = 0x0000000000000001   r11 = 0x0000000000000206
> 18:02:54     INFO -      r12 = 0x000000010f613000   r13 = 0x000000010f616b78
> 18:02:54     INFO -      r14 = 0x000000010f616a48   r15 = 0x000000010f613440
> 18:02:54     INFO -      rip = 0x00000001065076f2
> 18:02:54     INFO -      Found by: given as instruction pointer in context
> 18:02:54     INFO -   1  XUL!JSRuntime::~JSRuntime() [LinkedList.h:41855b016a72 : 328 + 0x5]
> 18:02:54     INFO -      rbx = 0x000000010f616b18   rbp = 0x00007fff5fbfeea0
> 18:02:54     INFO -      rsp = 0x00007fff5fbfee30   r12 = 0x000000010f613000
> 18:02:54     INFO -      r13 = 0x000000010f616b78   r14 = 0x000000010f616a48
> 18:02:54     INFO -      r15 = 0x000000010f613440   rip = 0x00000001064b59f5
> 18:02:54 INFO - Found by: call frame info

Unfortunately, this patch means that these leaks could definitely lead to UAF. Right now, that is just a maybe, depending on if the ScriptSource's data is in the compressed source set or not.

:-/
Depends on: 1261106
See Also: → 1219098
Attachment #8736047 - Attachment is obsolete: true
As per IRL discussion with :jimb, MOZ_ASSERT => MOZ_RELEASE_ASSERT in the case
where handles into the cache outlive the cache, since this leads to UAF 100% of
the time.
Attachment #8747888 - Flags: review+
Attachment #8747866 - Attachment is obsolete: true
More rebasing to fix bustage.
Attachment #8748262 - Flags: review+
Attachment #8747888 - Attachment is obsolete: true
Fix bad rebase on top of bug 1219098. Walked through it with jimb in person, so
carrying r+.
Attachment #8748419 - Flags: review+
Attachment #8748262 - Attachment is obsolete: true
As discussed in person today. Will fold into the main patch so as not to break
bisection.
Attachment #8749389 - Flags: review?(jimb)
No longer depends on: 1261106
Comment on attachment 8749389 [details] [diff] [review]
Hack around pre-existing leaks in the CC graph and avoid UAF

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

Looks good to me.

::: js/src/vm/Runtime.cpp
@@ +490,5 @@
> +
> +    // TODO bug 1261106: There is a pre-existing leak in Gecko's cycle collected
> +    // heap graph involving unknown edges holding the C++ half of an XHR object
> +    // alive (which then holds the JS reflector alive as well) that only happens
> +    // during shutdown on OSX marionette tests. The introduction of the

"during restart", right?

@@ +499,5 @@
> +    // text past the destruction of the `JSRuntime` and their owning cache,
> +    // which the cache asserts loudly against. I (fitzgen) spent a month
> +    // tracking someone else's leak down, and finally couldn't justify any more
> +    // time on it. Since it only happens during shutdown, and only in a very
> +    // niche scenario, I'm not *that* worried about it. So, if we detect that

You probably want to drop these two sentences; they're not really about the code. People looking at 1261106 will see the story.

@@ +503,5 @@
> +    // niche scenario, I'm not *that* worried about it. So, if we detect that
> +    // there are still active borrows, just leak the whole cache, and the OS
> +    // will clean everything up in a second anyways. I'm not happy with this
> +    // bandaid, but it is the pragmatic solution and it avoids potential
> +    // use-after-free.

This is probably the most important thing in the comment; consider leading with it.

@@ +562,5 @@
>  
>      rtSizes->mathCache += mathCache_ ? mathCache_->sizeOfIncludingThis(mallocSizeOf) : 0;
>  
>      rtSizes->sharedImmutableStringsCache +=
> +        sharedImmutableStrings_->sizeOfExcludingThis(mallocSizeOf);

Doesn't this want a "sizeOfIncludingThis" now that it's not part of some larger object?

::: js/src/vm/SharedImmutableStringsCache.h
@@ +348,5 @@
> +
> +#ifdef DEBUG
> +        // We can only use whether the table is empty or not as a synonym for
> +        // "are there active borrows?" if table entries are guaranteed never to
> +        // have a zero refcount.

Nice.
Attachment #8749389 - Flags: review?(jimb) → review+
Addresses review comments and folded back into main patch.
Attachment #8749431 - Flags: review+
Attachment #8748419 - Attachment is obsolete: true
Attachment #8749389 - Attachment is obsolete: true
Will fold this into the main patch after review to avoid breaking bisection.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22740c5a322b
Attachment #8749802 - Flags: review?(jimb)
Attachment #8749802 - Attachment is obsolete: true
Attachment #8749802 - Flags: review?(jimb)
OSX Marionette tests are green, ASAN tests are green. Looks very promising.
Comment on attachment 8749803 [details] [diff] [review]
Hack around shutdown craziness by refcounting the shared immutable strings cache's contents

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

::: js/src/jsscript.cpp
@@ -46,5 @@
>  #include "vm/Debugger.h"
>  #include "vm/Opcodes.h"
>  #include "vm/SelfHosting.h"
>  #include "vm/Shape.h"
> -#include "vm/SharedImmutableStringsCache.h"

I don't think we're supposed to delete the main #include just because we're #including the inline header.

::: js/src/threading/ExclusiveData.h
@@ +104,5 @@
> +     */
> +    template <typename... Args>
> +    explicit ExclusiveData(Args&&... args) {
> +        new (value_.addr()) T(mozilla::Forward<Args>(args)...);
> +    }

Reverence: "WHAT IS THIS BLACK MAGIC"
Reverent contempt:  "... AND HOW CAN I TREAT ITS EDGE CONDITIONS AS KEY FEATURES"

nm

::: js/src/vm/SharedImmutableStringsCache.h
@@ +184,5 @@
> +    SharedImmutableStringsCache clone() {
> +        MOZ_ASSERT(inner_);
> +        auto locked = inner_->lock();
> +        locked->refcount++;
> +        return SharedImmutableStringsCache(inner_);

I think it's pretty weird that  constructing an instance of the SharedImmutableStringsCache type, which exists to reference-count an ExclusiveData<Inner>, doesn't actually increment the reference count, and it's left to every use of the constructor to take care of that. Granted, that constructor is private.
Attachment #8749803 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #95)
> I think it's pretty weird that  constructing an instance of the
> SharedImmutableStringsCache type, which exists to reference-count an
> ExclusiveData<Inner>, doesn't actually increment the reference count, and
> it's left to every use of the constructor to take care of that. Granted,
> that constructor is private.

Yes, it is a little strange, but it is required because we sometimes have the lock held when invoking this constructor, and we must avoid reentrant locking attempts. As you point out, it is a private ctor, so not that bad all things considered.
https://hg.mozilla.org/mozilla-central/rev/98a28a1fce30
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Am currently having build faliures:

c:\t1\hg\objdir-sm\dist\include\js/HashTable.h(1135) : error C2280: 'js::SharedImmutableStringsCache::SharedImmutableStringsCache(const js::SharedImmutableStringsCache &)' : attempting to reference a deleted function
        c:\t1\hg\comm-central\mozilla\js\src\vm/SharedImmutableStringsCache.h(192) : see declaration of 'js::SharedImmutableStringsCache::SharedImmutableStringsCache'
        This diagnostic occurred in the compiler generated function 'js::SharedImmutableString::SharedImmutableString(const js::SharedImmutableString &)'
(In reply to Philip Chee from comment #100)
> Am currently having build faliures:

What compiler and version?
Visual Studio 2013 Community Edition.
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #103)
> Created attachment 8750460 [details] [diff] [review]
> follow up - Explicitly delete implicitly deleted copy constructors for VS2013

Philip: does this patch fix the build on VS2013 for you?
Flags: needinfo?(philip.chee)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #103)
> Created attachment 8750460 [details] [diff] [review]
> follow up - Explicitly delete implicitly deleted copy constructors for VS2013

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0088605bc505
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #104)
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #103)
> > Created attachment 8750460 [details] [diff] [review]
> > follow up - Explicitly delete implicitly deleted copy constructors for VS2013
> 
> Philip: does this patch fix the build on VS2013 for you?

No because I still fail at:
Bug 1264499
>      // If you want to copy, do it explicitly with `clone`.
> -    SharedImmutableStringsCache(const SharedImmutableStringsCache&) = delete;
> +    SharedImmutableStringsCache& operator=(const SharedImmutableStringsCache&) = delete;

This one works if I delete the second and third hunks.
Flags: needinfo?(philip.chee)
> This one works if I delete the second and third hunks.
The patch attached to this comment compiles with VS2013 but of course I don't know if it gives the correct result.
Attachment #8750689 - Flags: feedback?
Comment on attachment 8750689 [details] [diff] [review]
VS2013 safe patch

Oops. https://treeherder.mozilla.org/#/jobs?repo=try&revision=16b1621584a2
Attachment #8750689 - Flags: feedback? → feedback?(nfitzgerald)
Comment on attachment 8750689 [details] [diff] [review]
VS2013 safe patch

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

Strange that it doesn't like the deletion of the copy assignment operators... f+ I guess?

Waldo?
Attachment #8750689 - Flags: feedback?(nfitzgerald) → feedback+
Attachment #8750460 - Flags: review?(jwalden+bmo) → review+
I assume Waldo meant to r+ attachment 8750689 [details] [diff] [review] ?
Depends on: 1273962
You need to log in before you can comment on or make changes to this bug.