Bug 154512 – Added new port JSCOnly.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Konstantin Tokarev]

This port allows to build JavaScriptCore engine with minimal dependencies (right now only ICU is required).

[Konstantin Tokarev]

Created attachment 271880 [details]
Patch

[Michael Catanzaro]

Comment on attachment 271880 [details]
Patch

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

I think we should do this, it can only make it easier to contribute to JSC.

I'm concerned by the missing implementations of MainThread, RunLoop, and WorkQueue, though. Are those really not needed by JSC? I'd drop some ASSERT_NOT_REACHED() calls into WorkQueue::dispatch and WorkQueue::dispatchAfter, and RunLoop::dispatch in RunLoop.cpp (in an #ifdef). Maybe you'll find something important is not being run....

> Source/cmake/OptionsJSCOnly.cmake:22
> +if (CMAKE_MAJOR_VERSION LESS 3)

Nowadays I think everyone will have CMake 3 or higher... since this is only a convenience for developers (makes the build stop on error) we can probably get rid of it from OptionsGTK.cmake, and probably don't need it for this new port either.

> Source/cmake/OptionsJSCOnly.cmake:43
> +# You can build JavaScriptCore as a static library if you specify it as STATIC

Did you consider making this a public build option?

WEBKIT_OPTION_BEGIN()
WEBKIT_OPTION_DEFINE(ENABLE_STATIC_JSC "Whether to build JavaScriptCore as a static library." PUBLIC OFF)
WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PUBLIC ON)
WEBKIT_OPTION_END()

if (ENABLE_STATIC_JSC)
    set(JavaScriptCore_LIBRARY_TYPE STATIC)
endif ()

> Tools/Scripts/webkitdirs.pm:2038
> +    if ((isGtk() || isJSCOnly()) && -e "$buildPath/build.sh") {

(We could get rid of this code too. In a different patch, of course.)

[Konstantin Tokarev]

Comment on attachment 271880 [details]
Patch

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

>> Source/cmake/OptionsJSCOnly.cmake:22
>> +if (CMAKE_MAJOR_VERSION LESS 3)
> 
> Nowadays I think everyone will have CMake 3 or higher... since this is only a convenience for developers (makes the build stop on error) we can probably get rid of it from OptionsGTK.cmake, and probably don't need it for this new port either.

FWIW, I'm routinely using 2.8.12.2, but it won't be a dealbreaker if 3.0 becomes required.
Another option is to split cmake < 3 support into separate file and include it into both places.

>> Source/cmake/OptionsJSCOnly.cmake:43
>> +# You can build JavaScriptCore as a static library if you specify it as STATIC
> 
> Did you consider making this a public build option?
> 
> WEBKIT_OPTION_BEGIN()
> WEBKIT_OPTION_DEFINE(ENABLE_STATIC_JSC "Whether to build JavaScriptCore as a static library." PUBLIC OFF)
> WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PUBLIC ON)
> WEBKIT_OPTION_END()
> 
> if (ENABLE_STATIC_JSC)
>     set(JavaScriptCore_LIBRARY_TYPE STATIC)
> endif ()

Will do it.

[Michael Catanzaro]

(In reply to comment #3)
> FWIW, I'm routinely using 2.8.12.2, but it won't be a dealbreaker if 3.0
> becomes required.

Well it wouldn't be required, but you'd lose colors and pretty-printing, according to the comment.

But I see 2.8.12.2 is used by Ubuntu 14.04; makes sense to keep the current support a while longer, I think.

> Another option is to split cmake < 3 support into separate file and include
> it into both places.

Probably easiest to leave it be, since I plan to remove it eventually.

[Konstantin Tokarev]

Noot sure what "pretty-printing" is meant here; as for colors it's possible to get colored diagnostics by re-running ninja manually

[Konstantin Tokarev]

Created attachment 271881 [details]
Patch

[Csaba Osztrogonác]

(In reply to comment #4)
[snip]
> But I see 2.8.12.2 is used by Ubuntu 14.04; makes sense to keep the current
> support a while longer, I think.
[snip]

Ubuntu 14.04 is shipped with GCC 4.8 and this GCC isn't supported long time ago.
For example https://trac.webkit.org/changeset/196873 removed the GCC 4.8
workaround from bmalloc.

I tried this patch on r196872 and got another build error:
../../Source/JavaScriptCore/ftl/FTLLazySlowPathCall.h:51:1: warning: control reaches end of non-void function [-Wreturn-type]

FTLLazySlowPathCall.h was added by https://trac.webkit.org/changeset/190860
4 months ago and wasn't touched ever since then.

It seems GCC 4.8 build is completely broken and many workarounds should
be added to make it work again. Is there anybody who is interested to
do it?

I would say we should go ahead and try to forget GCC 4.8.

[Yusuke Suzuki]

(In reply to comment #7)
> (In reply to comment #4)
> [snip]
> > But I see 2.8.12.2 is used by Ubuntu 14.04; makes sense to keep the current
> > support a while longer, I think.
> [snip]
> 
> Ubuntu 14.04 is shipped with GCC 4.8 and this GCC isn't supported long time
> ago.
> For example https://trac.webkit.org/changeset/196873 removed the GCC 4.8
> workaround from bmalloc.
> 
> I tried this patch on r196872 and got another build error:
> ../../Source/JavaScriptCore/ftl/FTLLazySlowPathCall.h:51:1: warning: control
> reaches end of non-void function [-Wreturn-type]
> 
> FTLLazySlowPathCall.h was added by https://trac.webkit.org/changeset/190860
> 4 months ago and wasn't touched ever since then.
> 
> It seems GCC 4.8 build is completely broken and many workarounds should
> be added to make it work again. Is there anybody who is interested to
> do it?
> 
> I would say we should go ahead and try to forget GCC 4.8.

Yeah. Fortunately, in Ubuntu, we can introduce newer GCC (4.9) by using PPA. So throwing GCC 4.8 away does not block building the WebKit on Ubuntu 14.04 :)

[Konstantin Tokarev]

(In reply to comment #7)
> (In reply to comment #4)
> [snip]
> > But I see 2.8.12.2 is used by Ubuntu 14.04; makes sense to keep the current
> > support a while longer, I think.
> [snip]
> 
> Ubuntu 14.04 is shipped with GCC 4.8 and this GCC isn't supported long time
> ago.
> For example https://trac.webkit.org/changeset/196873 removed the GCC 4.8
> workaround from bmalloc.

I haven't tried building with r196873, but until yesterday GCC 4.8 worked fine for me for Linux/MIPS (no FTL). I cannot upgrade yet because Broadcom does not provide toolchains with newer GCC yet. My plan was to set up JSCOnly build slave for MIPS based using this GCC 4.8 toolchain to ensure that compilation with 4.8 is not broken.

I can try to fix FTL issues if it is required to officially allow building with 4.8.

[Csaba Osztrogonác]

It seems sampling profiler tests fail on JSCOnly port:
stress/sampling-profiler-anonymous-function.js
stress/sampling-profiler-basic.js
stress/sampling-profiler-bound-function-name.js
stress/sampling-profiler-deep-stack.js
stress/sampling-profiler-display-name.js
stress/sampling-profiler-internal-function-name.js

I don't think if it is a blocker issue, but it would
be great to investigate this bug in the near future.

[Konstantin Tokarev]

(In reply to comment #10)
> It seems sampling profiler tests fail on JSCOnly port:
> stress/sampling-profiler-anonymous-function.js
> stress/sampling-profiler-basic.js
> stress/sampling-profiler-bound-function-name.js
> stress/sampling-profiler-deep-stack.js
> stress/sampling-profiler-display-name.js
> stress/sampling-profiler-internal-function-name.js
> 
> I don't think if it is a blocker issue, but it would
> be great to investigate this bug in the near future.

I guess it is a result of using fake event loop. My current plan is to allow switching event loop via build option, as I would prefer to keep ability to use fake event loop.

I'm also considering writing loop using select(2) or poll(2), but I'm not sure how complex such code would become.

[Csaba Osztrogonác]

Comment on attachment 271881 [details]
Patch

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

> Source/WTF/wtf/none/MainThreadNone.cpp:1
> +#include "config.h"

Could you add copyright and license header to this new file?

> Source/WTF/wtf/none/RunLoopNone.cpp:1
> +#include "config.h"

ditto

> Source/WTF/wtf/none/WorkQueueNone.cpp:1
> +#include "config.h"

ditto

[Michael Catanzaro]

Comment on attachment 271881 [details]
Patch

I think it's better to take the time to get this right and land it with a working event loop.  (And copyright headers!)

[Konstantin Tokarev]

Created attachment 272458 [details]
Patch

[Konstantin Tokarev]

I've added optional dependency on glib: if it is found, glib event loop code is used. Other event loop variants (Mac, Win, Efl) may be added later.

I've replaced PLATFORM(GTK) with USE(GLIB) in WorkQueue.h, but I'm not completely satisfied with this solution. Efl also defines USE(GLIB), but it does not use glib event loop (and Qt port does the same when GStreamer is used), so result depends heavily on #if clause ordering. 

I think it would make sense to introduce special conditions for event loop code choice, e.g. USE(EVENTLOOP_GLIB), USE(EVENTLOOP_EFL), etc. What do you think?

[Michael Catanzaro]

Comment on attachment 272458 [details]
Patch

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

> Source/WTF/wtf/WorkQueue.h:43
> +#elif USE(GLIB)

Honestly, I think the way you have it here is best: use DispatchQueueEfl for EFL, otherwise use the GLib stuff. This isn't the only place we have such conditionals. I think it's nicer this way than introducing new USE(EVENTLOOP_GLIB) USE(EVENTLOOP_EFL) flags.

Anyway, changing from PLATFORM(GTK) to USE(GLIB) is definitely correct.

> Source/cmake/OptionsJSCOnly.cmake:28
> +    SET_AND_EXPOSE_TO_BUILD(USE_GLIB 1)

But I'm uncomfortable with autodetecting GLib support and silently disabling it if not present. This means some users will have a broken JSC due to the missing RunLoop/WorkQueue implementations, and others will not, even while building in exactly the same way, based on the presence of development files for some seemingly-random library.

One solution is to make this a public option, on by default, with a descriptive error message (without GLib, this and this will break) so users have to explicitly pass -DUSE_GLIB=OFF to CMake to build without it. I would r+ such a patch.

But even so, it seems like a broken vs. not broken option to me. Perhaps best to just require GLib for now, and work on a new WorkQueue implementation to remove the dependency later. GLib has very few dependencies and it's available everywhere; it would be nice to have no dependency on GLib, but I don't think it's so horrible.

[Konstantin Tokarev]

(In reply to comment #16)
> Comment on attachment 272458 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272458&action=review
> 
> > Source/WTF/wtf/WorkQueue.h:43
> > +#elif USE(GLIB)
> 
> Honestly, I think the way you have it here is best: use DispatchQueueEfl for
> EFL, otherwise use the GLib stuff. This isn't the only place we have such
> conditionals. I think it's nicer this way than introducing new
> USE(EVENTLOOP_GLIB) USE(EVENTLOOP_EFL) flags.

My concerns rehashed:

1. It is possible to make JSCOnly building with EFL loop, but it doesn't seem to be a good idea to define PLATFORM(EFL) in this case. Same goes for DARWIN and WINDOWS cases.

2. USE(GLIB) does not mean "Use GLib enent loop" in generic case

> 
> Anyway, changing from PLATFORM(GTK) to USE(GLIB) is definitely correct.
> 
> > Source/cmake/OptionsJSCOnly.cmake:28
> > +    SET_AND_EXPOSE_TO_BUILD(USE_GLIB 1)
> 
> But I'm uncomfortable with autodetecting GLib support and silently disabling
> it if not present. This means some users will have a broken JSC due to the
> missing RunLoop/WorkQueue implementations, and others will not, even while
> building in exactly the same way, based on the presence of development files
> for some seemingly-random library.
> 
> One solution is to make this a public option, on by default, with a
> descriptive error message (without GLib, this and this will break) so users
> have to explicitly pass -DUSE_GLIB=OFF to CMake to build without it. I would
> r+ such a patch.

I'll add option to choose event loop type by name, with GLib being default for now (later default should depend on OS).

> 
> But even so, it seems like a broken vs. not broken option to me. Perhaps
> best to just require GLib for now, and work on a new WorkQueue
> implementation to remove the dependency later. GLib has very few
> dependencies and it's available everywhere; it would be nice to have no
> dependency on GLib, but I don't think it's so horrible.

I don't agree with "broken vs. not broken" part. "None" is not broken, it is just not feature complete, e.g. all tests from run-layout-jsc pass.

[Michael Catanzaro]

> I'll add option to choose event loop type by name, with GLib being default
> for now (later default should depend on OS).

OK, in this case it makes sense to have USE(EVENTLOOP_GLIB) and USE(EVENTLOOP_EFL).

> I don't agree with "broken vs. not broken" part. "None" is not broken, it is
> just not feature complete, e.g. all tests from run-layout-jsc pass.

OK.

[Csaba Osztrogonác]

Konstantin, is there any chance to push JSCOnly port to trunk soon?

[Konstantin Tokarev]

Yes, I'm going to finish it today

[Csaba Osztrogonác]

(In reply to comment #20)
> Yes, I'm going to finish it today

Cool, good news. ;)

[Konstantin Tokarev]

Created attachment 273903 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 273903 [details]
Patch

Clearing flags on attachment: 273903

Committed r198086: <http://trac.webkit.org/changeset/198086>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

Nice!