Bug 72854 – [Mac] eliminate JavaScriptCore.exp

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Hajime Morrita]

This bug is for book-keeping. 
Once all visibile symbols are marked in the source code, we would no longer need .exp files.

[Hajime Morrita]

Created attachment 116039 [details]
Patch

[Hajime Morrita]

The patch is just FYI. We need other patches landed for this anyway.

[Mark Rowe (bdash)]

Comment on attachment 116039 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116039&action=review

> Source/JavaScriptCore/Configurations/Base.xcconfig:75
> +OTHER_CFLAGS_BASE = -fvisibility=hidden;

This is a roundabout way of expressing:
GCC_SYMBOLS_PRIVATE_EXTERN = YES;

[Hajime Morrita]

Created attachment 116065 [details]
Patch

[Hajime Morrita]

> This is a roundabout way of expressing:
> GCC_SYMBOLS_PRIVATE_EXTERN = YES;
Thanks again for your advice! 
I updated the patch. 
This patch also contains the symbol-hiding ld flags.

[Hajime Morrita]

Created attachment 122385 [details]
Patch

[Hajime Morrita]

Hi Mac build masters!

By Bug 72855, the export macros are in the tree for JSC. 
I think it's time to eliminate export files. Could you take a look?
I hope this be landed before another symbols appears in the list.

[Darin Adler]

Comment on attachment 122385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122385&action=review

What are the differences between what was in the .exp files and what is exported now? None at all?

> Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:37
> +OTHER_LDFLAGS_HIDE_SYMBOLS = -Wl,-unexported_symbol -Wl,__ZTISt9bad_alloc -Wl,-unexported_symbol -Wl,__ZTISt9exception -Wl,-unexported_symbol -Wl,__ZTSSt9bad_alloc -Wl,-unexported_symbol -Wl,__ZTSSt9exception -Wl,-unexported_symbol -Wl,__ZdlPvS_ -Wl,-unexported_symbol -Wl,__ZnwmPv;

Is this correct for both clang and gcc? How did you generate this list?

[Hajime Morrita]

Hi Darin, thanks for your interest!

(In reply to comment #8)
> (From update of attachment 122385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122385&action=review
> 
> What are the differences between what was in the .exp files and what is exported now? None at all?
> 
It should be same as the .exp lists since the JS_EXPORT_PRIVATE and WTF_EXPORT_PRIVATE is generated by these lists.

> > Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig:37
> > +OTHER_LDFLAGS_HIDE_SYMBOLS = -Wl,-unexported_symbol -Wl,__ZTISt9bad_alloc -Wl,-unexported_symbol -Wl,__ZTISt9exception -Wl,-unexported_symbol -Wl,__ZTSSt9bad_alloc -Wl,-unexported_symbol -Wl,__ZTSSt9exception -Wl,-unexported_symbol -Wl,__ZdlPvS_ -Wl,-unexported_symbol -Wl,__ZnwmPv;
> 
> Is this correct for both clang and gcc? How did you generate this list?
This is copied from WebKit2/Configurations/Base.xcconfig, following the advice by bdash at Bug 72862.

[Darin Adler]

(In reply to comment #9)
> (In reply to comment #8)
> > What are the differences between what was in the .exp files and what is exported now? None at all?
> > 
> It should be same as the .exp lists since the JS_EXPORT_PRIVATE and WTF_EXPORT_PRIVATE is generated by these lists.

Yes, I understand that is the theory.

But I was wondering about test results.

Are the lists identical? If not, what differences do you see? I suppose I could apply the patch myself and see what the differences are if you haven’t done it yet. If I was testing I’d probably use the “nm” command line tool.

[Hajime Morrita]

(In reply to comment #10)
> But I was wondering about test results.
> 
> Are the lists identical? If not, what differences do you see? I suppose I could apply the patch myself and see what the differences are if you haven’t done it yet. If I was testing I’d probably use the “nm” command line tool.

Ah, I got the point. Thanks for your clarification!
I'll try it then report it back here.

[Hajime Morrita]

Hi Darin,

I just tried. Here is the result: https://gist.github.com/1619569
And you are right - The number of exported symbols increases from 630 to 711.
The overhead is about 12% .

I found that the compiler generates two functions for each constructor or destructor
to handle virtual inheritance. That is the main cause of this overhead.

Here is a brief explanation about the topic: 
http://www.codeplay.com/company/documents/avoid-code-duplication-in-g-abi.html

The symbol list can control which one to export. But the code-level annotation cannot.
This is a limitation of this approach...

Although we wouldn't able to completely eliminate this overhead,
we can minimize the impact by
- A. Hiding constructors behind corresponding create() functions.
- B. Inlining constructors and destructors if possible.

"A" would be effective for polymorphic classes like JS* family.  "B" would work for WTF classes.
I think I can attack some of these. But I wonder how much overhead is acceptable.
The load time increase will be obviously less than linear.

I'd like to hear you and experts' opinion.

[Darin Adler]

I’m OK with the multiple constructor and destructor entry points being exported instead of just the one we need.

But I do see other differences that might be mistakes.

A) Why do these s_info objects need to be exported?

JSC::InterruptedExecutionError::s_info
JSC::JSBoundFunction::s_info
JSC::RegExpMatchesArray::s_info
JSC::RegExp::s_info

B) Why do these need to be exported?

WTF::charactersToDouble(unsigned char const*, unsigned long, bool*, bool*)
WTF::charactersToIntStrict(unsigned char const*, unsigned long, bool*, int)
WTF::charactersToUIntStrict(unsigned char const*, unsigned long, bool*, int)

WTF::String::number(short)
WTF::String::toIntPtrStrict(bool*, int) const
WTF::String::toUInt64Strict(bool*, int) const

C) Why do these "thunks" need to be exported?

__ZThn16_N3JSC9CodeBlockD0Ev
__ZThn16_N3JSC9CodeBlockD1Ev
_ceilThunk
_expThunk
_floorThunk
_jsRoundThunk
_logThunk

D) What abut these?

_getHostCallReturnValue
_operationGetByIdBuildList
_operationGetByIdOptimize
_operationGetByIdProtoBuildList
_operationLinkCall
_operationLinkConstruct
_operationPutByIdDirectNonStrictOptimize
_operationPutByIdDirectStrictOptimize
_operationPutByIdNonStrictOptimize
_operationPutByIdStrictOptimize

If we can cut down the differences further or justify these changes, then I think we’re ready to throw the switch.

[Simon Hausmann]

(In reply to comment #13)
> I’m OK with the multiple constructor and destructor entry points being exported instead of just the one we need.
> 
> But I do see other differences that might be mistakes.
[...]
> D) What abut these?
> 
> _getHostCallReturnValue
> _operationGetByIdBuildList
> _operationGetByIdOptimize
> _operationGetByIdProtoBuildList
> _operationLinkCall
> _operationLinkConstruct
> _operationPutByIdDirectNonStrictOptimize
> _operationPutByIdDirectStrictOptimize
> _operationPutByIdNonStrictOptimize
> _operationPutByIdStrictOptimize

I think these are due to missing HIDE_SYMBOL macro usages in dfg/DFGOperations.cpp for x64 and x64-64 builds.

[Darin Adler]

We should resolve as many of these as possible, and then revisit whether to throw the switch.

[Hajime Morrita]

Hi Darin, thank you for telling your opinion
and for taking careful investigation!
I took a look at your points.

* On A, the set of s_info: 

They are annotated as JS_EXPORTDATA, which is used for Windows port.
http://trac.webkit.org/search?q=JS_EXPORTDATA&changeset=on
Since this change pushes us to share exported symbols between ports,
they are exported even for Mac port.
I'm not sure why Windows port requires them to be exported though.

* On B, some string related methods:

They were annotated manually before the we applied the automated annotations.
I guess we can just remove it. I'll file a bug for that.

* On C and D, thunks.

They are defined in assembly forms (in jit/ThunkGenerator.cpp, dfg/DFGOperations.cpp), 
which don't have any visibility specifier. Thus they, I guess, go visible.
As Simon mentioned, there may be some way to hide it. 
But they are relatively harmless because 1) the number of symbols is small and 
2) It's hard to accidentally use them from outside.

My feeling is that we need to take care of B, but can keep A.
For C and D, we can eventually clear them, but it doesn't look serious blocker.

We can even tune the macro definition for tackling A. 
But it would be easier to attack after this change being landed
because then we can use our bots to try the modification.

What do you think?

[Darin Adler]

(In reply to comment #16)
> * On A, the set of s_info: 
> 
> They are annotated as JS_EXPORTDATA, which is used for Windows port.
> http://trac.webkit.org/search?q=JS_EXPORTDATA&changeset=on
> Since this change pushes us to share exported symbols between ports,
> they are exported even for Mac port.
> I'm not sure why Windows port requires them to be exported though.

My guess is that it doesn’t require them at this time, and either never did or did only in the past.

> * On B, some string related methods:
> 
> They were annotated manually before the we applied the automated annotations.
> I guess we can just remove it. I'll file a bug for that.

Seems fine to remove them. We can add them back later. I like the idea of not exporting things until needed since this is not public API, but rather used inside our project.

> * On C and D, thunks.
> 
> They are defined in assembly forms (in jit/ThunkGenerator.cpp, dfg/DFGOperations.cpp), 
> which don't have any visibility specifier. Thus they, I guess, go visible.
> As Simon mentioned, there may be some way to hide it. 
> But they are relatively harmless because 1) the number of symbols is small and 
> 2) It's hard to accidentally use them from outside.

To me it seems worthwhile to take a few moments to deal with this unless it is hard to do. Lets not give up without even mentioning something to the guys working on the JIT.

> My feeling is that we need to take care of B, but can keep A.

Is there a reason we can’t quickly test if resolving (A) causes any problems? I agree that if it does, we can back off, but if not it would be nice to get rid of it.

[Filip Pizlo]

(In reply to comment #16)

> * On C and D, thunks.
> 
> They are defined in assembly forms (in jit/ThunkGenerator.cpp, dfg/DFGOperations.cpp), 
> which don't have any visibility specifier. Thus they, I guess, go visible.
> As Simon mentioned, there may be some way to hide it. 
> But they are relatively harmless because 1) the number of symbols is small and 
> 2) It's hard to accidentally use them from outside.

The number of defined-in-assembly symbols on Mac will increase before it decreases.  See https://bugs.webkit.org/show_bug.cgi?id=75812.  We define several hundred things in asm via an external code generator. So having a good way of making things hidden by default makes that code really simple.

On the other hand, if there existed a consistent mechanism for marking things hidden through asm, and we can introduce some macros to do it across platforms, then this might not be a problem at all.

In particular, does HIDE_SYMBOL() not do what it claims to do?  On Mac it's just:

#define HIDE_SYMBOL(name) ".private_extern _" #name

[Hajime Morrita]

Hi Darin and Filip,
Thanks for your feedback!

> > They are annotated as JS_EXPORTDATA, which is used for Windows port.
> > http://trac.webkit.org/search?q=JS_EXPORTDATA&changeset=on
> > Since this change pushes us to share exported symbols between ports,
> > they are exported even for Mac port.
> > I'm not sure why Windows port requires them to be exported though.
> 
> My guess is that it doesn’t require them at this time, and either never did or did only in the past.

OK, so I'll try to remove it - Filed Bug 77145.
If it breaks the build. I'll close it as invalid.

> 
> > * On B, some string related methods:
> > 
> > They were annotated manually before the we applied the automated annotations.
> > I guess we can just remove it. I'll file a bug for that.
> 
> Seems fine to remove them. We can add them back later. I like the idea of not exporting things until needed since this is not public API, but rather used inside our project.
> 

Totally agree with this point.
My guess is that the these annotations are kind of historical artifact from past experiment.
Let me remove these: Bug 77113.

> > * On C and D, thunks.
> > 
> > They are defined in assembly forms (in jit/ThunkGenerator.cpp, dfg/DFGOperations.cpp), 
> > which don't have any visibility specifier. Thus they, I guess, go visible.
> > As Simon mentioned, there may be some way to hide it. 
> > But they are relatively harmless because 1) the number of symbols is small and 
> > 2) It's hard to accidentally use them from outside.
> 
> To me it seems worthwhile to take a few moments to deal with this unless it is hard to do. Lets not give up without even mentioning something to the guys working on the JIT.
> 

Yeah, as Filip kindly pointed out, it looks better to attack these
especially because something cool is happening in new JSC world.
Filed Bug 77150 for this.
Shamelessly to say, I'm not good at asm-related stuff like this.
So maybe I'll ask you guys to help.

[Simon Hausmann]

(In reply to comment #18)
> (In reply to comment #16)
> 
> > * On C and D, thunks.
> > 
> > They are defined in assembly forms (in jit/ThunkGenerator.cpp, dfg/DFGOperations.cpp), 
> > which don't have any visibility specifier. Thus they, I guess, go visible.
> > As Simon mentioned, there may be some way to hide it. 
> > But they are relatively harmless because 1) the number of symbols is small and 
> > 2) It's hard to accidentally use them from outside.
> 
> The number of defined-in-assembly symbols on Mac will increase before it decreases.  See https://bugs.webkit.org/show_bug.cgi?id=75812.  We define several hundred things in asm via an external code generator. So having a good way of making things hidden by default makes that code really simple.
> 
> On the other hand, if there existed a consistent mechanism for marking things hidden through asm, and we can introduce some macros to do it across platforms, then this might not be a problem at all.
> 
> In particular, does HIDE_SYMBOL() not do what it claims to do?  On Mac it's just:
> 
> #define HIDE_SYMBOL(name) ".private_extern _" #name

Yeah, HIDE_SYMBOL works as expected AFAIK. In the case of this bug it was just missing in the X86 and X86-64 branches in DFGOperations.cpp. I uploaded a patch to hide these.

[Hajime Morrita]

(In reply to comment #20)
> > In particular, does HIDE_SYMBOL() not do what it claims to do?  On Mac it's just:
> > 
> > #define HIDE_SYMBOL(name) ".private_extern _" #name
> 
> Yeah, HIDE_SYMBOL works as expected AFAIK. In the case of this bug it was just missing in the X86 and X86-64 branches in DFGOperations.cpp. I uploaded a patch to hide these.
Great! Thanks much for taking this, Simon.

[Hajime Morrita]

By the way,
> __ZThn16_N3JSC9CodeBlockD0Ev
> __ZThn16_N3JSC9CodeBlockD1Ev
These are generated by the compiler for parts of CodeBlock destructor.
It looks we need these extra symbols because CodeBlock has multiple inheritance.
http://tinydrblog.appspot.com/?p=89001

[Hajime Morrita]

Created attachment 125047 [details]
Patch

[Hajime Morrita]

Hi Darin,
After clearing some unintentionally exported symbols,
the number of exported symbols is now reduced from 711 to 690.
Here is the latest diff: https://gist.github.com/1715443

I wish this to land before another new symbols are slipping in,
So it would be appreciated if you take a look at this.

Thanks,

[WebKit Review Bot]

Attachment 125047 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': timed out waiting for server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hajime Morrita]

> Updating OpenSource
> RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': timed out waiting for server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295
This is a false alarm. it's just flaky.

[Darin Adler]

The only symbol in here that raises my eyebrows is this:

__ZNK3JSC11Interpreter24retrieveCallerFromVMCodeEPNS_9ExecStateEPNS_10JSFunctionE

[Darin Adler]

Comment on attachment 125047 [details]
Patch

Lets throw the switch!

[Gavin Barraclough]

Woohoo! - this is awesome.

[Hajime Morrita]

Committed r106606: <http://trac.webkit.org/changeset/106606>

[Hajime Morrita]

Landed! Thanks much for you help Darin and you guys who did help!

>__ZNK3JSC11Interpreter24retrieveCallerFromVMCodeEPNS_9ExecStateEPNS_10JSFunctionE
I'll take a look.

[Hajime Morrita]

(In reply to comment #31)
> >__ZNK3JSC11Interpreter24retrieveCallerFromVMCodeEPNS_9ExecStateEPNS_10JSFunctionE
> I'll take a look.
Removed: http://trac.webkit.org/changeset/106614
My git blame told me that this added manually, maybe just to follow the code around it.