889543 - Use AutoCycleDetector for CloneMemory in vm/SelfHosting.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Currently, json.cpp is using a custom built CycleDetector class. It could easily make use of the AutoCycleDetector in jscntxt.h, which provides several optimizations and automatic marking over what is used in json.cpp.

Comment 1

[Till Schneidereit [:till]]

Does that also apply to CloneObject in SelfHosting.cpp?

Comment 2

[Terrence Cole [:terrence]]

Thanks, Till. It seems it could indeed by used there. I updated the json.cpp case while fixing a fuzz bug, so lets repurpose this bug to the CloneObject case.

Comment 3

[Terrence Cole [:terrence]]

Attachment: autocycledetect_for_clonememory-v0.diff

The existing CloneMemory mechanism reifies cycles by returning the already cloned object, instead of simply failing when it hits a cycle. This surprised me as I didn't think our current self-hosted primitives (or any primitives, for that matter) have a cycle. To check, I ran both test suites with an assertion instead of reification. Nothing failed, so I think it is probably fine to remove this capability and instead fail in the presence of cycles.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8389228 [details] [diff] [review]
autocycledetect_for_clonememory-v0.diff

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

Yeah, I guess we don't really need cycles in any self-hosted object graphs. However, the code should absolutely report an error in case there is a cycle instead of just silently failing: we've had too many instances of silent failure in self-hosted code already, making it pretty hard to debug at times.

Also, is the cycle checking expensive? If so, can we maybe only do it in debug builds? All self-hosted code should have good test coverage, and we're not dealing with dynamic objects, generally.

All of this is conditional on something I'm not 100% sure about: Intl not generating cyclic graphs. f?'ing waldo for verification of that.

(Also, sorry for the late review, vacation and all that ...)

::: js/src/vm/SelfHosting.cpp
@@ +1001,5 @@
> +    AutoCycleDetector detect(cx, selfHostedObject);
> +    if (!detect.init())
> +        return nullptr;
> +    if (detect.foundCycle())
> +        return nullptr;

As mentioned above, this absolutely must dump an error instead of failing silently.

@@ +1130,3 @@
>      }
> +
> +    gc::AutoSuppressGC suppress(cx);

Is this needed with the other changes in place? Seems to me like cloning should be GC-safe now, no?

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8389228 [details] [diff] [review]
autocycledetect_for_clonememory-v0.diff

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

Yeah, I don't think Intl does anything cycle-y, particularly not in self-hosting startup/init phases.

Comment 6

[Terrence Cole [:terrence]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Firefox Commit Bot]

The following changeset is now in Firefox Nightly:

> d29c145b465c Bug 889543 - Use AutoCycleDetector instead of CloneMemory; r=till

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central