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

Hook spidermonkey into rust's allocator #72

Closed
wants to merge 2 commits into from

Conversation

@michaelwu
Copy link
Contributor

michaelwu commented Feb 12, 2016

This is the alternative to #61 . Rust doesn't quite provide all the APIs necessary to do this fully - there is no calloc equivalent, but it's probably fine in most cases.

fake_rustalloc is provided to link the js shell and tests, but isn't used anywhere else.

@asajeffrey - do you mind seeing if this works as well as turning on jemalloc?

r? @Manishearth

Review on Reviewable

$(SRC_DIR)/mozjs/js/src/configure $(strip $(CONFIGURE_FLAGS))
cd $(OUT_DIR) && make -f Makefile -j$(NUM_JOBS)
endif

LDFLAGS := $(OUT_DIR)/fake_rustalloc.o
$(OUT_DIR)/fake_rustalloc.o: rust_mem/fake_rustalloc.cpp
$(CXX) $(CPPFLAGS) $(CXXFLAGS) $< -o $@ -c

This comment has been minimized.

@Manishearth

Manishearth Feb 12, 2016

Member

Where do we link this to Rust's jemalloc?

This comment has been minimized.

@michaelwu

michaelwu Feb 12, 2016

Author Contributor

We don't - this is only used to make standalone binaries link successfully in SM. Otherwise, tests and js shell don't link.

This comment has been minimized.

@Manishearth

Manishearth Feb 12, 2016

Member

So when will things link to Rust's jemalloc?

This comment has been minimized.

@michaelwu

michaelwu Feb 12, 2016

Author Contributor

To elaborate a bit more - fake_rustalloc is used for linking C++ binaries that are only used for testing. These binaries are built by SM using a C++ compiler. In these cases, fake_rustalloc just hooks these binaries up to the system allocator. In normal cases however, fake_rustalloc isn't used and rustc properly links SM against rust's allocator.

This comment has been minimized.

@Manishearth

Manishearth Feb 12, 2016

Member

Ah, so the cpp and header file below are only used for testing? In case we're using rustc, it will automatically link against jemalloc's malloc/calloc/realloc symbols? Sounds good.

So what's the situation right now? If the linking happens in the final link step, why is SM not using jemalloc (which is being linked against) now?

This comment has been minimized.

@michaelwu

michaelwu Feb 12, 2016

Author Contributor

SM is currently using js_malloc/js_realloc/js_free, which redirects to the system malloc/realloc/free. Rust's copy of jemalloc uses je_malloc/jerealloc/je_free naming, so it doesn't automatically override other allocators.

This comment has been minimized.

@Manishearth

Manishearth Feb 12, 2016

Member

(figured it out after chatting in IRC; only fake_rustalloc is for testing, jscustomallocator is not)

static inline void* js_calloc(size_t bytes)
{
void* buf = __rust_allocate(bytes, 0);
if (bytes && buf) {

This comment has been minimized.

@Manishearth

Manishearth Feb 12, 2016

Member

could we call je_calloc here?

This comment has been minimized.

@michaelwu

michaelwu Feb 12, 2016

Author Contributor

(discussed on IRC) rust doesn't always bundle jemalloc.

@Manishearth
Copy link
Member

Manishearth commented Feb 12, 2016

r=me once @asajeffrey gets a chance to play with it

@asajeffrey
Copy link
Member

asajeffrey commented Feb 13, 2016

I'll have a look at this tomorrow.

@asajeffrey
Copy link
Member

asajeffrey commented Feb 14, 2016

Oh okay, maybe the day after tomorrow.

@asajeffrey
Copy link
Member

asajeffrey commented Feb 19, 2016

Got a segfault (posted to irc, crossposting here for posterity):

#0  0x0000555557123576 in je_sdallocx ()
#1  0x0000555556c44c11 in js_free (p=0x0) at /home/ajeffrey/github/servo/mozjs/rust_mem/jscustomallocator.h:62
#2  js::ScopedFreePtrTraits<unsigned char>::release (ptr=0x0) at ../../dist/include/js/Utility.h:303
#3  mozilla::Scoped<js::ScopedFreePtrTraits<unsigned char> >::~Scoped (this=<synthetic pointer>, __in_chrg=<optimized out>) at ../../dist/include/mozilla/Scoped.h:107
#4  js::ScopedJSFreePtr<unsigned char>::~ScopedJSFreePtr (this=<synthetic pointer>, __in_chrg=<optimized out>) at ../../dist/include/js/Utility.h:305
#5  js::NewStringCopyNDontDeflate<(js::AllowGC)0, unsigned char> (cx=cx@entry=0x7fffccc87000, s=s@entry=0x555557376370 <js_CollatorCompareGet_str> "Intl_Collator_compare_get", 
    n=n@entry=25) at /home/ajeffrey/github/servo/mozjs/mozjs/js/src/vm/String.cpp:1116
#6  0x0000555556c44d15 in js::NewStringCopyN<(js::AllowGC)0, unsigned char> (cx=cx@entry=0x7fffccc87000, 
    s=s@entry=0x555557376370 <js_CollatorCompareGet_str> "Intl_Collator_compare_get", n=n@entry=25) at /home/ajeffrey/github/servo/mozjs/mozjs/js/src/vm/String.cpp:1150
#7  0x0000555556a8d570 in AtomizeAndCopyChars<unsigned char> (ib=js::InternAtom, length=25, tbchars=0x555557376370 <js_CollatorCompareGet_str> "Intl_Collator_compare_get", 
    cx=0x7fffccc87000) at /home/ajeffrey/github/servo/mozjs/mozjs/js/src/jsatom.cpp:350
#8  js::Atomize (cx=cx@entry=0x7fffccc87000, bytes=0x555557376370 <js_CollatorCompareGet_str> "Intl_Collator_compare_get", length=25, ib=ib@entry=js::InternAtom)
    at /home/ajeffrey/github/servo/mozjs/mozjs/js/src/jsatom.cpp:425
#9  0x0000555556a8e284 in JSRuntime::initializeAtoms (this=this@entry=0x7fffccc40000, cx=cx@entry=0x7fffccc87000) at /home/ajeffrey/github/servo/mozjs/mozjs/js/src/jsatom.cpp:147
#10 0x0000555556e6e433 in js::NewContext (rt=0x7fffccc40000, stackChunkSize=<optimized out>) at /home/ajeffrey/github/servo/mozjs/mozjs/js/src/jscntxt.cpp:125
#11 0x000055555670da30 in rust::Runtime::new::h98f4f7f5d95588c8dCi ()
#12 0x00005555560df03b in script_thread::ScriptThread::new_rt_and_cx::h5ff56114013021957sj ()
#13 0x00005555562b2bf9 in script_thread::ScriptThread::new::h0d84950678ab1af1vqj ()
#14 0x00005555562b0111 in sys_common::unwind::try::try_fn::h15090099631519579870 ()
#15 0x0000555557111a5c in __rust_try ()
#16 0x000055555710e06e in sys_common::unwind::inner_try::hf5df2c554f3d837dFdt ()
#17 0x00005555562b1b7b in boxed::F.FnBox$LT$A$GT$::call_box::h6716255501184128001 ()
#18 0x0000555557113e84 in sys::thread::Thread::new::thread_start::h05e6a3d5b147b01bsCx ()
#19 0x00007ffff5c206aa in start_thread (arg=0x7fffcdbff700) at pthread_create.c:333
#20 0x00007ffff573eeed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
@michaelwu
Copy link
Contributor Author

michaelwu commented Feb 19, 2016

It turns out the allocator API that rust exposes and the one SM wants are more incompatible than expected. Going to see if I can make the SM allocator API fully sized. If so, we can come back to hooking SM into Rust's allocator after the next smup.

@michaelwu
Copy link
Contributor Author

michaelwu commented Feb 29, 2016

rust-lang/rust#31976 adds support for unsized deallocation.

@perlun
Copy link

perlun commented May 31, 2016

@michaelwu - has this gone stale? Will it be finished?

@metajack
Copy link
Contributor

metajack commented May 31, 2016

@perlun I believe that @michaelwu will not be continuing work on this. I would love it if someone saw this through to completion though.

@perlun
Copy link

perlun commented Jun 1, 2016

@metajack - alright. I don't think I feel confident about doing it (yet), but maybe it would be worth for you to enumerate what you believe to be the remaining steps? So someone could more easily bring it forward.

@metajack
Copy link
Contributor

metajack commented Jun 1, 2016

@perlun I think the first step is to check if the Rust landscape changed since this was opened.

Rust doesn't quite provide all the APIs necessary to do this fully - there is no calloc equivalent, but it's probably fine in most cases.

and later

t turns out the allocator API that rust exposes and the one SM wants are more incompatible than expected. Going to see if I can make the SM allocator API fully sized.

Is that still true or likely to change soon? Maybe the first thing to do to answer those question is to write down what this API is on the SM side so we can starting discussing how to get there.

cc @asajeffrey @nikomatsakis @tschneidereit

@tschneidereit
Copy link

tschneidereit commented Dec 23, 2016

@fitzgen, this is something we should also have on the radar. We need either this or #61 for SpiderMonkey to be at its best.

@fitzgen
Copy link
Member

fitzgen commented Dec 23, 2016

From #61:

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

I find this really surprising since the GC allocates in 2MiB chunks and then uses its own allocator, but I suppose there is still a lot of vm code that is using normal malloc.


Anyways, I haven't read through this backlog, but here is the status WRT allocators for my current smup work. I ported from the custom makefile majiggery to using js/src/devtools/autospider.py to drive the builds. Using the same infrastructure that firefox / sm / m-c's CI is using is a good thing on principle and means we'll have less breakage since there are more eyes on it. I'm not passing any configuration flags or options to autospider.py, so that means we will have the equivalent of #61: a dupicate copy of jemalloc.

I can look into sharing the same jemalloc some more once I complete the smup.

@fitzgen
Copy link
Member

fitzgen commented Dec 23, 2016

I find this really surprising since the GC allocates in 2MiB chunks and then uses its own allocator, but I suppose there is still a lot of vm code that is using normal malloc.

And array elements are backed by direct mallocs as well, and that seems to be the case that the benchmark is hitting.

@nox
Copy link
Member

nox commented Oct 8, 2019

Many things changed since 2016, on both SpiderMonkey and Rust sides, closing this because I don't think it can be salvaged.

@nox nox closed this Oct 8, 2019
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.