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

Enabled jemalloc. #61

Closed
wants to merge 1 commit into from
Closed

Enabled jemalloc. #61

wants to merge 1 commit into from

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Nov 3, 2015

The --disable-jemalloc flag in makefile.cargo is replaced by --enable-jemalloc.

The memory library is loaded (as discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1134039).

This speeds up SM performance significantly (about 2x on some Dromaeo JS tests).

Review on Reviewable

The --disable-jemalloc flag in makefile.cargo is replaced by --enable-jemalloc.

The memory library is loaded (as discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1134039).

This speeds up SM performance significantly (about 2x on some Dromaeo JS tests).
@michaelwu
Copy link
Contributor

michaelwu commented Nov 3, 2015

Can you figure out how to hook up SM to rust's allocator? Rust's allocator is jemalloc.

@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 3, 2015

Sharing an allocator between servo and SM would be good, it should reduce allocator overhead and memory fragmentation.

@metajack
Copy link
Contributor

metajack commented Nov 3, 2015

@@ -1,4 +1,4 @@
CONFIGURE_FLAGS := --disable-jemalloc
CONFIGURE_FLAGS := --enable-jemalloc

This comment has been minimized.

Copy link
@tschneidereit

tschneidereit Nov 7, 2015

``--enable-jemalloc` shouldn't have any effect, so I think this line can just be removed.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Nov 7, 2015

Author Member

Yes, it is redundant, but I included it to be explicit.

@waddlesplash
Copy link

waddlesplash commented Nov 9, 2015

This speeds up SM performance significantly (about 2x on some Dromaeo JS tests).

In the screenshots from https://github.com/servo/servo/wiki/Benchmarking#core-js-performance - old and new, that does appear to be the case in general, but for "Array Construction, new Array()" performance has fallen to ~15% of what it was before...? This is on par with Firefox's performance, but it doesn't seem to make a lot of sense that this new allocator is faster on everything but that...

@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 10, 2015

Yes, it is a bit odd. I think production code makes more use of [] than new Array (), but still there's something of going on here.

@tschneidereit
Copy link

tschneidereit commented Nov 10, 2015

@jandem, do you have any idea why enabling jemalloc might cause a regression in new Array() performance (at least as measured by Dromaeo)? Seems like this might be relevant to Firefox, too.

@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 10, 2015

@tschneidereit OK, here's a theory... The Array [] test is running:

    for ( var j = 0; j < i * 10; j++ )
        ret = new Array(i);

Each array is being initialized at a different size, so jemalloc is going to a different area of memory for each of them (to avoid fragmentation). Malloc doesn't try to avoid fragmentation, so is just allocating memory sequentially. This gives malloc better cache performance than jemalloc.

Note that the other tests initialize an empty array with no backing store.

This is just a theory, but it would explain the results.

@tschneidereit
Copy link

tschneidereit commented Nov 10, 2015

After IRC discussions, I filed bug 1223373 for this.

@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 10, 2015

D'oh, ignore my theory, I misread new Array(i) as new Array(j). The arrays are all being allocated as size 1024. This may be stress-testing the gc more than anything else.

@glandium
Copy link

glandium commented Nov 10, 2015

Enabling jemalloc from SM as used by servo is likely to have unwanted consequences due to how it's being setup on the different platforms. On Linux, it's likely to have no effect because it requires libmozglue.a to be linked into any produced executable, which is probably not happening for Servo. On Mac, it's likely to hijack rust's allocator.

@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 11, 2015

Are you saying that having two separate allocators for SM and servo is likely to be safer?

@Manishearth
Copy link
Member

Manishearth commented Nov 11, 2015

Rust uses jemalloc too, shouldn't break much.

Rust has (experimental) pluggable allocators.IIRC we will probably instruct it to use SM's allocator directly in all cases.

@glandium
Copy link

glandium commented Nov 11, 2015

I'm saying the patch is not going to do what the intent is. Said differently, making SM use jemalloc in servo is not as simple as removing --disable-jemalloc.

@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 11, 2015

Well the intention is to get the servo running times on pure JS down to bring comparable to FF. Experimentally, the problem is jemalloc. What do we need to do to enable jemalloc in SM over and above removing --disable-jemalloc?

@glandium
Copy link

glandium commented Nov 11, 2015

I'd say --disable-jemalloc is not the right base for that. You should look at what it takes to make malloc/free from SM call the rust allocator.

@Manishearth
Copy link
Member

Manishearth commented Nov 11, 2015

We should be (and plan to) doing it the other way around, like I said. Use SM's jemalloc instead since there's a documented way to plug allocators into Rust.

@glandium
Copy link

glandium commented Nov 11, 2015

Then you need something else than what --enable-jemalloc currently does, because --enable-jemalloc has a strong implication on how things are done, and assumes everything is handled by the gecko build system, and that it's fine to do things how they are done. Those assumptions fall short when building a rust application.

@Manishearth
Copy link
Member

Manishearth commented Nov 11, 2015

@michaelwu thoughts on what we should do here? We can try to make Rust use SM's allocator (there's a way of doing this), or make SM use Rust's allocator (not sure how this can be done, but it might be easier)

@michaelwu
Copy link
Contributor

michaelwu commented Nov 11, 2015

@asajeffrey
Copy link
Member Author

asajeffrey commented May 18, 2018

Closing since it seems that we need a different approach.

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

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