Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assorted SpiderMonkey GC-related fixes #6180

Closed
wants to merge 2 commits into from
Closed

Assorted SpiderMonkey GC-related fixes #6180

wants to merge 2 commits into from

Conversation

@jdm
Copy link
Member

jdm commented May 25, 2015

These changes combined allow running a debug-mozjs Servo build with JS_GCZEAL=2,20 (ie. perform a GC every 20 allocations).

r? @Ms2ger

Review on Reviewable

jdm added 2 commits May 25, 2015
…n to minimize the number of allocations that can occur before the global is ready to be traced.
@highfive
Copy link

highfive commented May 25, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented May 25, 2015

Critic review: https://critic.hoppipolla.co.uk/r/5096

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jdm
Copy link
Member Author

jdm commented May 25, 2015

@glennw These changes made the jQuery test suite hum along without incident for me.

@glennw
Copy link
Member

glennw commented May 25, 2015

@jdm Fantastic! Thank you for looking into it! :)

@michaelwu
Copy link
Contributor

michaelwu commented May 26, 2015

If #6150 can also run the jQuery test suite, I would prefer avoiding these changes. RootedVecs are fixed a bit differently and the issue with the global is part of a deeper issue that affects all dom objects being created/wrapped. The dom object being wrapped needs to be rooted while the wrapper is being setup because the dom object might point to other dom objects that aren't rooted.

@Ms2ger
Copy link
Contributor

Ms2ger commented May 30, 2015

So when is Void::reflector called?

@nox
Copy link
Member

nox commented May 30, 2015

@Ms2ger AFAIK, in the loop over dom_collections in RootedCollectionSet::trace().

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 1, 2015

If we're going to do this, I'd like to split up Void into one for storing in RootedVec and one for JS.

@metajack
Copy link
Contributor

metajack commented Jun 2, 2015

After discussion with @jdm he split off #6247 for the stuff that makes jquery test suite work. This will remain open for now until after the SM upgrade lands and we can look at what's still useful.

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 20, 2015

Please rebase on master.

@metajack
Copy link
Contributor

metajack commented Jul 29, 2015

JS upgrade landed. Is any of this still useful?

@jdm
Copy link
Member Author

jdm commented Jul 29, 2015

I suspect no.

@jdm jdm closed this Jul 29, 2015
@jdm jdm deleted the jdm:zealfixes branch Aug 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.