Compact Byte Code parser and executor for Jerry (release branch). #813
Conversation
|
Hi all, this pull request contains the release branch of a Compact Byte Code (CBC) parser and executor of Jerry. The short summary is we achieved progress on all aspects of Jerry, including memory consumption, performance and binary size. The bad news is that the patch is huge, but the good news we don't need to fix everything before landing it. It is enough to focus on the style issues. The rest can be done in followup patches. The biggest problem is loosing the change log after squashing, and this makes tracking bugs quite difficult. In the followings we show the results of the CBC work. Full Test262 conformance in debug mode with 30s timeout (in debug some tests runs several minutes, they would pass, but excluded now). === Summary ===
This shows that the CBC engine is stable, and there is no risk of moving to CBC. Of course the remaining issues will be fixed in (much) smaller commits. Binary size:
Parsing results of the concatenation of IoT.js on Raspberry Pi 2: Current master:
CBC Parser: Heap stats: Note: Total byte code size is 12650 bytes
Gain:
SunSpider results:
Only the runtime of one test is decreased, but everything else is improved. There are several tests with 25+ % memory or performance improvement. We did not focus on SunSpider results, it seems this is the effect of using a simpler design. I think we achieved more than we initially expected. The key advantage of Compct Byte Code is combining simplicity and efficiency, and this is proved by reducing both memory and binary size, and improved performance. Please review the patch and help us with feedback. Lets land this work soon and continue the development on the master branch. |
|
👍 |
|
Great! hope to be landing well shortly! :) |
|
@huxinx if you have some time, could you measure this patch on intel? |
|
There are build errors on X86. I just did a rough fix to make compiling pass. cbc_integration_dev_release...huxinx:cbc_integration_dev_release Run ./tools/run-perf-test.sh 10 times,
Mon Jan 25 07:47:57 EST 2016 Binary size: |
|
The numbers are too similar in the two columns. Are you sure you run the two different binaries? |
|
And please measure them on the edison board. |
|
on Edison Board
|
|
Update SunSpider results on NUC. @zherczeg |
|
Thank you very much! |
|
Regarding failures, this patch is quite big (1.3Mb at the moment), so it will likely cause breaks. We will fix these issues. Perhaps not everything in this huge patch, but in follow-up patches. Anyway, we just achieved 98.1% test262 conformance in debug mode, and preparing a new release patch soon. |
zherczeg
force-pushed
the
cbc_integration_dev_release
branch
from
75207b5 to
d3c75d1
Compare
|
Release patch is done. Latest results on RPi2:
|
|
@zherczeg, good job! We have several questions:
|
|
Hi,
|
|
I'd argue against keeping both implementations in the code base. That would give only a little advantage over keeping the two implementations in branches/forks but will be a hell to maintain, and will certainly not point toward a best unified solution. I'm much concerned that the two byte code approaches will keep living their own more and more divergent life. |
zherczeg
force-pushed
the
cbc_integration_dev_release
branch
from
d3c75d1 to
fec49f5
Compare
|
Sry, accidentally closing this. |
|
I strongly agree with @akiss77 and share his concerns. I don't think it makes sense to have two different byte code implementations in the tree. @sand1k @egavrin @ruben-ayrapetyan In the last IoT.js/JerryScript workshop all parties agreed that if the compact byte code turns out to be a significant improvement it will replace the existing implementation. Those goals have been met so we should proceed accordingly. Making both byte code implementations coexist is a significant amount of effort with no perceived gain. This effort is better spent on improving JerryScript. The snapshot support is an important feature but it should not block the merge of the compact byte code. The current patch is big enough as it is and as there are no (known) use cases where the lack of snapshotting causes a regression (the overall benefit of snapshotting is also reduced due to the increased efficiency of the compact byte code implementation). @lemmaa , you seem to be very keen on getting the compact byte code merged, what is your perspective on this? |
|
@tilmannOSG I really want to land this patch ASAP. At the same time, I also think that we would better to keep both implementations, at least for a while. Because we need to have enough time to investigate CBC implementations from every perspective. So, how about keep both implementations until next workshop and do investigation and improvement? One big problem in this approach is I can't find what is efficient way to keep the both. Build time configuration looks too complex to make patch, and keep in separate branch also in maintenance. From the other point of view, I think we need to consider making followings completed before landing.
As for snapshot, CBC reduces footprint of byte code very big, but main purpose of snapshot is not reducing the footprint of byte code but moving byte code storage to FLASH from RAM. So It need to be implemented even CBC landed. One more, the effect of snapshot is much higher in large framework codes, like @zherczeg, @egavrin Do you have good idea to keep both implementations? |
|
@lemmaa AFAIK
@zherczeg can confirm the above (and give an estimation when the next PR update from the dev branch is planned to happen) |
|
tests-262, jerry-release:
The 12 failures are also part of the 20, so new failures introduced.
The difficulty for having multiple implementations in one trunk:
I fully understand the risk of moving to a new implementation. The question is whether all my tools will work? Please try them and we will fix the issues you encounter in a short time. I think maintaining two implementations is much more resource consuming than fixing the issues. We can also check out the current byte code from git if we need it in the future. For example, we have already been aware that one line needs to be changed in IoT.js for cbc: there is no need to include the zero terminator for strings passed to jerry_eval (zero is not a valid JavaScript character, and CBC correctly throws a syntax error). After that change, all IoT.js tests are passed. But you might encounter other issues as well. Perhaps we should open an IoT.js branch for cbc, and collect such fixes there. |
zherczeg
force-pushed
the
cbc_integration_dev_release
branch
from
a6977f3 to
3d8075c
Compare
|
New commit with snapshot support. "make precommit" runs cleanly. Please try it and let us know if something does not work. |
|
Yes, as mentioned before, having two byte code implementations in the tree is an absolute maintenance nightmare. This is not a practical way to move forward. JerryScript is a small project and we need to be careful to spend our limited resources on efforts which add significant value to the project. As @zherczeg said, there's always a risk in moving to a new implementation. However, our current results indicate that the compact byte code implementation is a strict improvement both in correctness and performance. In addition, the authors of this patch already have a track record of addressing any identified issues quickly. Moving to the new byte code implementation is also fully transparent to the users. This pull request has been open for one and a half weeks now and to my knowledge there was not a single issue reported. Everyone who has concerns about this patch please test it with your code and report any issues you found. @huxinx If you could test the new implementation with your own use cases we would highly appreciate any feedback :) I propose that we allow for another week of testing and assuming no blocking issues are found we proceed with the merge. |
|
@akiss77 @zherczeg Thanks for explanation, it looks like all issues cleared for me. :) Today, @wateret added @egavrin We need your opinion about this, please. |
|
Thank you @lemmaa. Progress is visible on those charts as well. |
zherczeg
force-pushed
the
cbc_integration_dev_release
branch
from
3d8075c to
5f45684
Compare
|
We tried IoT.js with snapshot. It works after the following two changes:
Both changes are unrelated to CBC. |
|
Latest results on NUC. OS, ubuntu 15.04, 32 bit
Tue Feb 2 06:29:51 EST 2016 |
|
Thank you for the measurement. I suppose that means the compilation issues you encountered before were fixed. I hope @sand1k @egavrin @ruben-ayrapetyan give their positive feedback soon and we can land this four month of work. I am not aware of anything else that blocks the landing process. |
|
Since no serious issues has been reported, we would like to land this patch tomorrow. If anybody encounters any bugs please report them and we will fix them. Please report only the bugs. I am aware that in a 40000 lines of patch there is a lot of opportunity to refactor certain parts, but that would delay the landing of this major advancement. Of course we can (should!) refactor those parts in smaller patches later. |
JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com JerryScript-DCO-1.0-Signed-off-by: Tamas Gergely tgergely.u-szeged@partner.samsung.com JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély zsborbely.u-szeged@partner.samsung.com JerryScript-DCO-1.0-Signed-off-by: Roland Takacs rtakacs.u-szeged@partner.samsung.com JerryScript-DCO-1.0-Signed-off-by: István Kádár ikadar@inf.u-szeged.hu JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com
zherczeg
force-pushed
the
cbc_integration_dev_release
branch
from
5f45684 to
876bd62
Compare
|
Landed in 4d2dd22 |
Compact Byte Code parser and executor for Jerry.
JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg@inf.u-szeged.hu
JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com
JerryScript-DCO-1.0-Signed-off-by: Tamas Gergely tgergely.u-szeged@partner.samsung.com
JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély zsborbely.u-szeged@partner.samsung.com
JerryScript-DCO-1.0-Signed-off-by: Roland Takacs rtakacs.u-szeged@partner.samsung.com
JerryScript-DCO-1.0-Signed-off-by: István Kádár ikadar@inf.u-szeged.hu