Last Comment Bug 736963 - Move jemalloc'ed strdup/strndup definitions
: Move jemalloc'ed strdup/strndup definitions
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on:
Blocks: jemalloc3
  Show dependency treegraph
 
Reported: 2012-03-19 03:22 PDT by Mike Hommey [:glandium]
Modified: 2012-03-31 19:17 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Move jemalloc'ed strdup/strndup definitions (4.67 KB, patch)
2012-03-19 08:52 PDT, Mike Hommey [:glandium]
justin.lebar+bug: review+
Details | Diff | Review
Move jemalloc'ed strdup/strndup definitions (4.68 KB, patch)
2012-03-28 06:14 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Review
Move jemalloc'ed strdup/strndup definitions (4.69 KB, patch)
2012-03-28 06:15 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Review
Move jemalloc'ed strdup/strndup definitions (4.69 KB, patch)
2012-03-28 06:18 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Review

Description Mike Hommey [:glandium] 2012-03-19 03:22:07 PDT
I want us to stop adding stuff directly in jemalloc code, which will allow easier jemalloc upgrades in the future. This bug is about strdup/strndup.
Comment 1 Mike Hommey [:glandium] 2012-03-19 08:52:46 PDT
Created attachment 607179 [details] [diff] [review]
Move jemalloc'ed strdup/strndup definitions

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=9363f5544207
Comment 2 Justin Lebar (not reading bugmail) 2012-03-27 09:39:55 PDT
Comment on attachment 607179 [details] [diff] [review]
Move jemalloc'ed strdup/strndup definitions

I have no idea if the build changes are right, but moving the definitions is fine.  I don't know if you want to get a review on the build changes from khuey.

Could we name the new file something other than jemalloc.c?
Comment 3 Mike Hommey [:glandium] 2012-03-27 10:32:01 PDT
(In reply to Justin Lebar [:jlebar] from comment #2)
> Could we name the new file something other than jemalloc.c?

Any suggestions?

I'll probably rename memory/src to memory/build, too.
Comment 4 Justin Lebar (not reading bugmail) 2012-03-27 10:36:29 PDT
(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to Justin Lebar [:jlebar] from comment #2)
> > Could we name the new file something other than jemalloc.c?
> 
> Any suggestions?

extraMallocFuns.c?
Comment 5 Mike Hommey [:glandium] 2012-03-28 06:14:46 PDT
Created attachment 610104 [details] [diff] [review]
Move jemalloc'ed strdup/strndup definitions
Comment 6 Mike Hommey [:glandium] 2012-03-28 06:15:36 PDT
Created attachment 610105 [details] [diff] [review]
Move jemalloc'ed strdup/strndup definitions
Comment 7 Mike Hommey [:glandium] 2012-03-28 06:18:22 PDT
Created attachment 610108 [details] [diff] [review]
Move jemalloc'ed strdup/strndup definitions
Comment 8 Ted Mielczarek [:ted.mielczarek] 2012-03-28 06:44:42 PDT
Comment on attachment 610108 [details] [diff] [review]
Move jemalloc'ed strdup/strndup definitions

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

::: Makefile.in
@@ +79,5 @@
>  endif
>  
>  ifdef MOZ_MEMORY
>  tier_base_dirs += memory/jemalloc
> +tier_base_dirs += memory/build

Just put these both in one assignment with a continuation.

::: mozglue/build/Makefile.in
@@ +48,5 @@
>  DIST_INSTALL = 1
>  
>  ifdef MOZ_MEMORY
> +SHARED_LIBRARY_LIBS = $(call EXPAND_LIBNAME_PATH,memory,$(DEPTH)/memory/build)
> +SHARED_LIBRARY_LIBS += $(call EXPAND_LIBNAME_PATH,jemalloc,$(DEPTH)/memory/jemalloc)

Single assignment with contuation here too.
Comment 9 Mike Hommey [:glandium] 2012-03-31 00:28:50 PDT
(In reply to Ted Mielczarek [:ted] from comment #8)
> Comment on attachment 610108 [details] [diff] [review]
> Move jemalloc'ed strdup/strndup definitions
> 
> Review of attachment 610108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: Makefile.in
> @@ +79,5 @@
> >  endif
> >  
> >  ifdef MOZ_MEMORY
> >  tier_base_dirs += memory/jemalloc
> > +tier_base_dirs += memory/build
> 
> Just put these both in one assignment with a continuation.
> 
> ::: mozglue/build/Makefile.in
> @@ +48,5 @@
> >  DIST_INSTALL = 1
> >  
> >  ifdef MOZ_MEMORY
> > +SHARED_LIBRARY_LIBS = $(call EXPAND_LIBNAME_PATH,memory,$(DEPTH)/memory/build)
> > +SHARED_LIBRARY_LIBS += $(call EXPAND_LIBNAME_PATH,jemalloc,$(DEPTH)/memory/jemalloc)
> 
> Single assignment with contuation here too.

I did neither when landing, because in both cases, we're soon going to have ifdef/endif between both lines.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2a9fb626038
Comment 10 Ed Morley [:emorley] 2012-03-31 19:17:19 PDT
https://hg.mozilla.org/mozilla-central/rev/c2a9fb626038

Note You need to log in before you can comment on or make changes to this bug.