Refactoring jerry-libc to squeeze out some extra bytes from the code (+fixes) #845

akosthekiss · 2016-02-07T22:29:41Z

'cause every byte matters :)

akosthekiss · 2016-02-07T22:40:26Z

The result and effect of the three patches on code size as applied on top of master and incrementally on each other. There is almost always some bytes or some houndred bytes gain, except for 3 hardly explicable code size increases.

executable	master	mem&str	gain	printf	gain	rnd	gain
debug.linux-cp/jerry	684307	684180	127	683853	454	683945	362
debug.linux-cp_minimal/jerry	470308	470181	127	469854	454	469882	426
debug.linux/jerry	877392	877265	127	876938	454	877030	362
debug.linux-mem_stats/jerry	882099	881972	127	882493	-394	882585	-486
debug.linux-mem_stress_test/jerry	877412	877285	127	876958	454	877050	362
debug.mcu_stm32f4-cp/jerry	855572	855372	200	855364	208	855452	120
debug.mcu_stm32f4-cp_minimal/jerry	504588	504436	152	504328	260	504408	180
release.linux-cp/jerry	218809	218747	62	218681	128	218681	128
release.linux-cp_minimal/jerry	135597	135519	78	135477	120	135477	120
release.linux/jerry	289829	289962	-133	289049	780	289049	780
release.linux-mem_stats/jerry	291124	290382	742	290316	808	290316	808
release.linux-mem_stress_test/jerry	290240	289714	526	289875	365	289875	365
release.mcu_stm32f3-cp/jerry	142656	142640	16	142632	24	142632	24
release.mcu_stm32f3-cp_minimal/jerry	81864	81840	24	81824	40	81824	40
release.mcu_stm32f4-cp/jerry	142656	142640	16	142632	24	142632	24
release.mcu_stm32f4-cp_minimal/jerry	81864	81840	24	81824	40	81824	40

LaszloLango · 2016-02-08T08:38:15Z

jerry-libc/jerry-libc.c

@@ -225,12 +202,12 @@ strncmp (const char *s1, const char *s2, size_t n)
 char * __attr_used___ // FIXME


Is this FIXME comment valid after your patch?

LaszloLango · 2016-02-08T08:40:24Z

LGTM

zherczeg · 2016-02-08T09:57:49Z

jerry-libc/jerry-libc-printf.c

-      value = (uintmax_t)va_arg (*args_list_p, int); /* short int is promoted to int */
-      break;
-    }
-


I think the compiler do this automatically.

Me too. Still, as the table above shows, doing the merge manually does have an impact on the code size.

Generally, I think it is good practice not to write overly verbose, or suboptimal, or clone-full code and rely on the compiler to optimize it away. Since sometimes it fails. (Up to a certain point of course. Extreme brevity and incomprehensible optimizations can have a critical maintenance cost in the long run, but that's another story. And this is not the case here, IMHO.)

akosthekiss · 2016-02-08T09:59:22Z

@LaszloLango I have to be honest, I don't know why some of the functions are tagged with the used attribute while others are not. I did not dare to touch it...

The description of the attribute is: "This attribute, attached to a function, means that code must be emitted for the function even if it appears that the function is not referenced."

zherczeg · 2016-02-08T10:00:46Z

jerry-libc/jerry-libc.c

@@ -263,7 +240,8 @@ strlen (const char *s)
 int
 rand (void)
 {
-  uint32_t intermediate = libc_random_gen_state[0] ^ (libc_random_gen_state[0] << 11);
+  uint32_t intermediate = libc_random_gen_state[0];
+  intermediate ^= intermediate << 11;


This is harder to me to read. And I think the compiler can optimize this as well.

The body contains two more ^=s... And as for the compiler optimization, same as above.

I think this is the only thing needs to be changed, and after that LGTM.

zherczeg · 2016-02-08T10:03:12Z

As for me I am not conviced it is worth to refactor the code this much. Perhaps in a few complicated cases.

akosthekiss · 2016-02-08T10:28:12Z

@zherczeg We are aiming at a super light libc, aren't we?
(Btw, the pr fixes 2 bugs. And I wouldn't like fixing the one in strncmp by keeping the current style. And if that changes, then the other funcs should be changed as well.)

akosthekiss · 2016-02-17T20:58:46Z

This PR has been pending for a while. What shall happen to it? And should it be rejected, what should happen to the issues it fixes?

LaszloLango · 2016-02-18T07:26:22Z

@akiss77, I'm still thinking of this change is needed, but @zherczeg had doubts. It depends on him.

zherczeg · 2016-02-18T07:47:04Z

jerry-libc/jerry-libc.c

@@ -284,5 +260,8 @@ rand (void)
 void
 srand (unsigned int seed) /**< new seed */
 {
+  libc_random_gen_state[0] =
+  libc_random_gen_state[1] =
+  libc_random_gen_state[2] =


Isn't this increase code size?

Most probably it is increasing the code size with some bytes. However, I cannot think of a smaller solution for fixing srand. Since that function must reinitialize the whole state of the pseudo-RNG to ensure that always the same sequence is generated from the same seed.

zherczeg · 2016-02-18T07:48:34Z

I think in general the patch is ok, but there are a few changes I don't particularly like. But I have no strong objection.

akosthekiss · 2016-02-18T08:17:14Z

@zherczeg, are the changes those you mentioned before, or something else? I could try and come up with "nicer" approaches.

Refactor memory and string handling routines * Potential code size improvements: * Most loops don't really need a new incrementing index variable running between 0..n-1 but can just loop `while (n--)`, and the bodies don't need array indexing but can just use the `*p++` idiom. * Compare routines are not required to return -1, 0, and +1, but any negative, zero, or positive result will do. * `strncmp` may follow the other routines and does not have to have defined behaviour if any of its args is NULL. * Fix: * `strncmp` did not work correctly if the strings were equal but `n` was greater than their length (did not stop at the terminating zero character). Refactor printf * Merging code duplications, removing dead initialization. Refactor rand and srand * Making sure that the type of the state variables is OK (`uint32_t` instead of `unsigned int`). * Getting type conversions OK. * Fixing `srand` to write all state variables and thus indeed generate the same random sequence. JerryScript-DCO-1.0-Signed-off-by: Akos Kiss akiss@inf.u-szeged.hu

akosthekiss added bug Undesired behaviour enhancement An improvement binary size Affects binary size labels Feb 7, 2016

LaszloLango reviewed Feb 8, 2016
View reviewed changes

LaszloLango assigned ruben-ayrapetyan Feb 8, 2016

zherczeg reviewed Feb 8, 2016
View reviewed changes

LaszloLango assigned zherczeg and unassigned ruben-ayrapetyan Feb 9, 2016

zherczeg reviewed Feb 18, 2016
View reviewed changes

akosthekiss force-pushed the libc-refactor branch from 103d5aa to f216f90 Compare February 18, 2016 11:06

akosthekiss force-pushed the libc-refactor branch from f216f90 to 3608f8d Compare February 18, 2016 14:43

akosthekiss merged commit 3608f8d into master Feb 18, 2016

akosthekiss deleted the libc-refactor branch February 18, 2016 15:18

zherczeg mentioned this pull request Apr 7, 2016

Nominating Akos Kiss (akiss77) for JerryScript Maintainer status #992

Closed

somang-park unassigned zherczeg Nov 25, 2016

		@@ -225,12 +202,12 @@ strncmp (const char s1, const char s2, size_t n)
		char * __attr_used___ // FIXME

Refactoring jerry-libc to squeeze out some extra bytes from the code (+fixes) #845

Refactoring jerry-libc to squeeze out some extra bytes from the code (+fixes) #845

Uh oh!

Conversation

akosthekiss commented Feb 7, 2016

Uh oh!

akosthekiss commented Feb 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaszloLango commented Feb 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosthekiss commented Feb 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Feb 8, 2016

Uh oh!

akosthekiss commented Feb 8, 2016

Uh oh!

akosthekiss commented Feb 17, 2016

Uh oh!

LaszloLango commented Feb 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Feb 18, 2016

Uh oh!

akosthekiss commented Feb 18, 2016

Uh oh!

Uh oh!