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

Xplat static library improvement v3 #2138

Closed

Conversation

kphillisjr
Copy link
Contributor

This should greatly improve the static library handling/generation on Ubuntu 16.04 LTS, and OS X. I believe that the Force_Load option should help reduce the symbol issues on with OS X regardless of version.

Note: I have not tested these changes on OS X where ChakraCore is stored in a path with spaces, but the fix for this should be fairly easy.

V2: changes: attempt to fix compile/runtime errors with MSVC/OSX, and errors on native tests.
V3: Dropped conversion of existing static libraries to objects.

This forces all symbols of each of the static libraries to load into
libChakraCore.dylib. This will increase runtime size a little, but
should hopefully prevent linker errors.
This will greatly reduce time required to rebuild and switch between
static and shared libraries.

Note: for Xcode/IOS, the force_load parameter is only good for the
next library mentioned. This patch forces all of the existing
libraries to get loaded into a single library libChakraCoreStatic,
and hopefully also will make it to where libChakraCore has all the
required symbols too.
Cmake currently does not allow for merging of multiple static libraries
together unless someone uses objects. This should greatly improve
Not all platforms work well with case insensitive file paths. This fixes
runtime on Linux partitions.
@msftclas
Copy link

msftclas commented Dec 3, 2016

Hi @kphillisjr, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

A minor typo lead to the compile error. This should resolve the issue.
This should fix the Native linker targets for xplat.
@kphillisjr
Copy link
Contributor Author

@obastemur - Just as a heads up, Even though OSX failed this time, the current status has great improvement. The logs show that this is failing at the runtime tests.

This should help prevent errors by ensuring that the complete static
library is loaded and that no typos exist in the arguments.
There was an extra parenthisis in one of the cmake files that lead to build
failure on OSX.
In future versions of CMake the leading/trailing whitespace will cause
errors since the behavior is deprecated.
This merges Chakra.Jsrt into libChakraCoreStatic, and adds a new static
library for GCStress called Chakra.Jsrt.Singular. This new library is
simply the old method for linking Chakra.Jsrt under a new name.
@kphillisjr
Copy link
Contributor Author

The next couple of pushes will merge the various static libraries into the core libChakraCoreStatic library.

This object is Consumed by the various other static libraries, and
reduces the number of overall libraries lying around the build directory.
This gets rid of the Chakra.Pal object all together.
@obastemur
Copy link
Collaborator

@kphillisjr We could use ar on linux and libtool on OSX to combine those three into one. However, we had concerns with the ordering of global constructors and destructors so we didn't do it yet. IMO, we don't have much good reasons to do it or push it.

So basically we could merge them without making these changes. (but we shouldn't until we are in a better shape with global cons etc.)

Second; this PR tries to combine the interfaces for shared and static libs under ChakraCore folder / project. This possibly improves the compile times for when someone compiles shared after static lib vice versa ? Please point me out if I'm missing other improvements and why this particular scenario is so important?

Could you please share how this approach would help with our shared lib initialization problem ?

As I mentioned many times; I really appreciate the effort but as long as there is no really good reason, I would go with ar / libtool (but not for now) and make final ChakraCore lib lighter instead.

@kphillisjr
Copy link
Contributor Author

kphillisjr commented Dec 5, 2016

@obastemur Wrote: This possibly improves the compile times for when someone compiles shared after static lib vice versa?

This is a Definite yes, it greatly reduces the build time to switch between static and shared libraries... Actually right now due to the changes I made here, the total compile time to change from static build to shared build and back in a single go is under 30 seconds because the only item that gets rebuilt is the ch utility.

Actually, with the build time being this small, it is possible to generate both a static ch utility ( say, ch.static ) and the regular ch utility, and have runtests.sh test both utilities one after another.

Please point me out if I'm missing other improvements and why this particular scenario is so important?

The other improvement by this approach is that there is less chance of bugs sneaking in by switching between static and shared libraries on xplat. Also, this fixes issue #1811 all together since the static symbol used internally by the static library is not defined out on xplat.

Could you please share how this approach would help with our shared lib initialization problem ?

The new file ChakraCoreCommon.cpp includes all of the symbols common between the static and shared libraries of chakracore. The VALIDATE_ENTER_CURRENT_THREAD() function takes care of the shared library Initialization for us in a well tested fashion... This is actually used in the static library already.

Edit: I I went ahead and ran a few tests to see how the resulting files looked under this system and here is a short table showing how things fared on Ubuntu 16.04 LTS.

File File Size Build Type
ch ( Shared Library) 1.8M Release
ch ( Static Linking) 17M Release
libChakraCore.so 17M Release
libChakraCoreStatic.a 46M Release
ch ( Shared Library) 3.1M Debug
ch ( Static Linking) 163M Debug
libChakraCore.so 173M Debug
libChakraCoreStatic.a 669M Debug

The build commands for these where...

#Note: All of these are ran on Ubuntu 16.04 LTS (amd64)
./build.sh -n
./build.sh -n --static
./build.sh -n --debug
./build.sh -n --debug --static  

Edit 2: After some testing, it appears that using ar / libtool have no change on produced files... Instead if you want to reduce the file size you need to run the strip utility.

@kphillisjr
Copy link
Contributor Author

kphillisjr commented Dec 8, 2016

I would like to get this Integrated into the 2.0 release window for pull #2170. What changes would be needed to merge these changes. The biggest Improvements this offers is as follows...

  • Generates a static library and shared library during initial run of build.sh.
  • Reduces compile definition pollution when --static argument is passed to build.sh. This provides dramatic build time improvements for older systems where recompiling everything could take more than 10 minutes.
  • Simpler linker requirements for xplat static builds - End users just need the system library dependencies, and ChakraCoreStatic to handle everything.
  • Simpler linker requirements for shared xplat builds - All you need to do is link to ChakraCore dynamic library, and this will automatically handle process initialization/termination on xplat.
  • Simpler packaging requirements for shared/static libraries - For linux the X86_64 prebuilt binaries compressed in tar.gz format that contains the Headers, static library and shared library is about 13MB for Release builds, and for Debug builds it is about 175mb. If you only include only the Shared Library, and headers the archive is about 5MB for Release builds and about 55MB for Debug builds.

@kphillisjr
Copy link
Contributor Author

@obastemur, I went ahead and rebased all of my changes on a new branch located at kphillisjr/ChakraCore at xplat_improvements_for_v2. These Changes are to address most of the problems designated in issue #1616.

@bjconlan
Copy link

I've been using the work done which integrates this merge (along with those in #1616 ) and can verify this is working for me in a 3rd party static library capacity on linux x64 debug. (And is a little more idiomatic than the current cmake targets defined for a/the static library/libraries)

@obastemur
Copy link
Collaborator

#2575 (comment)

@obastemur obastemur closed this Mar 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants