Refactor ecma value to store pointers directly in ecma values rather than compressing them. #1006

zherczeg · 2016-04-14T08:34:50Z

Ecma value is 32 bit long, and only 3 bits are used for type and flags. The rest is enough for encoding 8 byte aligned pointers without compressing them.

zherczeg · 2016-04-14T08:36:47Z

                      Benchmark |                   Perf(+ is better)
                     3d-cube.js |   2.408s ->   2.148s : +10.797%  (+-0.919%) : [+]
         access-binary-trees.js |   1.226s ->   1.130s :  +7.830%  (+-0.848%) : [+]
             access-fannkuch.js |   6.980s ->   6.280s : +10.029%  (+-1.087%) : [+]
                access-nbody.js |   2.628s ->   2.334s : +11.187%  (+-1.035%) : [+]
    bitops-3bit-bits-in-byte.js |   1.564s ->   1.370s : +12.404%  (+-0.632%) : [+]
         bitops-bits-in-byte.js |   2.314s ->   1.984s : +14.261%  (+-0.642%) : [+]
          bitops-bitwise-and.js |   3.136s ->   2.890s :  +7.844%  (+-0.541%) : [+]
          bitops-nsieve-bits.js |  13.238s ->  12.804s :  +3.278%  (+-1.217%) : [+]
       controlflow-recursive.js |   0.920s ->   0.780s : +15.217%  (+-0.000%) : [+]
                  crypto-aes.js |   3.088s ->   2.872s :  +6.995%  (+-0.742%) : [+]
                  crypto-md5.js |   2.602s ->   2.472s :  +4.996%  (+-1.309%) : [+]
                 crypto-sha1.js |   1.750s ->   1.614s :  +7.771%  (+-1.002%) : [+]
           date-format-tofte.js |   1.936s ->   1.812s :  +6.405%  (+-1.259%) : [+]
           date-format-xparb.js |   0.780s ->   0.710s :  +8.974%  (+-0.000%) : [+]
                 math-cordic.js |   2.710s ->   2.402s : +11.365%  (+-1.071%) : [+]
           math-partial-sums.js |   1.472s ->   1.314s : +10.734%  (+-0.948%) : [+]
          math-spectral-norm.js |   1.380s ->   1.170s : +15.217%  (+-0.000%) : [+]
               string-base64.js |   4.894s ->   4.714s :  +3.678%  (+-0.522%) : [+]
                string-fasta.js |   2.878s ->   2.668s :  +7.297%  (+-1.300%) : [+]
                Geometric mean: |                   Speed up: 9.344% (+-0.223%) : [+]

Binaries:
jerry_a09c473_rel_refactor_obj_prop: 183,748
jerry_41eb163_rel_pointer_encoding: 173,732

Overall: 10% speedup and 10K binary reduction.

LaszloLango · 2016-04-15T07:37:36Z

jerry-core/ecma/base/ecma-globals.h

@@ -58,7 +58,8 @@ typedef enum
  ECMA_TYPE_SIMPLE, /**< simple value */
  ECMA_TYPE_NUMBER, /**< 64-bit integer */
  ECMA_TYPE_STRING, /**< pointer to description of a string */
-  ECMA_TYPE_OBJECT /**< pointer to description of an object */
+  ECMA_TYPE_OBJECT, /**< pointer to description of an object */
+  ECMA_TYPE___MAX = ECMA_TYPE_OBJECT /** highest value for ecma types */


AFAIK, the common suffix is __COUNT instead of __MAX.

I introduced this form some time ago and use it nowadays. It is useful for switches.

There is 36 matches for __COUNT and 6 matches for __MAX in the current source base. I'd prefer to use only one of them, because they mean the same. But this could be in a followup patch. Leave as is, the code looks good to me. :)

Just a clarification comment: __MAX is the highest possible value, __COUNT is the number of possible values. If an enum is numbered from 0, as usual, then __COUNT == __MAX + 1. However, if enum labels have "arbitrarily" set values, then either __MAX or __COUNT may make no sense to use.

LaszloLango · 2016-04-15T11:22:12Z

LGTM, if @akiss77 has no objection, you can land it.

akosthekiss · 2016-04-15T12:20:14Z

jerry-core/ecma/base/ecma-helpers-value.c

-                                            ECMA_VALUE_VALUE_POS,
-                                            ECMA_VALUE_VALUE_WIDTH);
-} /* ecma_get_value_value_field */
+  if (sizeof (void *) <= sizeof (ecma_value_t))


This should rather be a compile-time decision. We've had similar before: #976 .

#if UINTPTR_MAX <= UINT32_MAX

That would be the equivalent, if I'm right. (A comment might be added pointing out that ecma_value_t is a typedef to uint32_t.)

This IS a compile time decision. I don't understand this.

There is a slight difference between if (0) and #if 0. You wrote the former, I prefer the latter.

if (0) can be optiized away (and usually is, I admit) but still has to be valid code (e.g., it must reference existing structure fields, which may not be there because of preproc guards used elsewhere -- not applicable to this case), while #if 0 certainly is dead code that the complier does not even see. That's why the latter is used normally.

Even in the current code base, we (almost) never decide on type sizes with C code, but (almost) always with preprocessor conditionals. I've worked my way through 250+ occurrences of sizeof in jerry-core, and the only exception I've found is ecma-helpers-external-pointers.c, which uses if (sizeof (x) < sizeof (y)) style. Which it shouldn't either, IMO.

And we don't need special comments. Compilers are clever enough to evaluate constants inside ifs/loops. E.g. while(true) is optimized with unconditional branches, and not load true into a register, and branch if that register is true. Or do {} while(false) is eliminated.

Here the code is valid in both cases. I worry that we might change the type of ecma_value_t and forget about this.

My problem with ECMA_VALUE_MAX is that it suggests that ecma value is a number. In reality it is a bit structure stored in a word sized value. I would like to store small integer values in ecma_value_t in the future, and would like to reserve MAX and MIN stuff for that purpose.

Then go for the second alternative for now.

What about the following define

#define ECMA_VALUE_CAN_HOLD_POINTERS

This define could be used for other purposes, like encoding internal pointer properties. They also don't need compresiion on 32 bit systems.

Could you add a commit on top of the current one to show how it would look like?

zherczeg · 2016-04-15T13:11:06Z

Patch updated.

akosthekiss · 2016-04-15T14:14:50Z

Much nicer to my eyes.
LGTM FWIW

…than compressing them. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com

LaszloLango added enhancement An improvement performance Affects performance memory consumption Affects memory consumption binary size Affects binary size labels Apr 14, 2016

LaszloLango added this to the Engine optimization & enhancement milestone Apr 14, 2016

LaszloLango reviewed Apr 15, 2016
View reviewed changes

zherczeg force-pushed the pointer_encoding_in_32_bit_mode branch from 41eb163 to 17e062b Compare April 15, 2016 10:55

akosthekiss reviewed Apr 15, 2016
View reviewed changes

zherczeg force-pushed the pointer_encoding_in_32_bit_mode branch from 17e062b to dc2a51b Compare April 15, 2016 13:10

Refactor ecma value to store pointers directly in ecma values rather …

e92ecd4

…than compressing them. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com

zherczeg force-pushed the pointer_encoding_in_32_bit_mode branch from ce84b35 to e92ecd4 Compare April 15, 2016 18:40

zherczeg merged commit e92ecd4 into jerryscript-project:master Apr 15, 2016

zherczeg deleted the pointer_encoding_in_32_bit_mode branch April 15, 2016 18:41

Refactor ecma value to store pointers directly in ecma values rather than compressing them. #1006

Refactor ecma value to store pointers directly in ecma values rather than compressing them. #1006

Uh oh!

Conversation

zherczeg commented Apr 14, 2016

Uh oh!

zherczeg commented Apr 14, 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!

Choose a reason for hiding this comment

Uh oh!

LaszloLango commented Apr 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosthekiss Apr 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Apr 15, 2016

Uh oh!

akosthekiss commented Apr 15, 2016

Uh oh!

Uh oh!

LaszloLango commented Apr 15, 2016 •

edited

Loading

akosthekiss Apr 15, 2016 •

edited

Loading