Issue 147 - [Typed objects] Implement object types

This is only the first type - so far only the actual object types have been implemented and no garbage collection mechanism.

[René Jeschke, 2014-04-21 17:24:12]

First: sorry for the long wait, needed to wrap my head around this and was
interrupted by a lot of 
other stuff regularly. 

I've not found that much, here are a few additional points that I have in my
mind:

Alignment:
    I was at first sceptical about alignment issues until I saw the 'sort' in
'defineProperties',
    so those are met.

Memory overhead per buffer:
    Another issue I see is that we should be aware of the (memory) overhead per
TypedObject buffer 
    and use buffers as large as possible to not undo the benefits of this
aproach.

Bit manipulation:
    There are two code pieces that use hard-to-understand bit manipulation
tricks, namely: 

    utils.js/ceilLog2:

        A replacement for the log2/ceilLog2 functions could look like:

        function intLog2(x)
        {
            return 1 << ~~Math.floor(Math.log(x) / Math.log(2));
        }

        function ceilIntLog2(x)
        {
            return 1 << ~~Math.ceil(Math.log(x) / Math.log(2));
        }

        ... on a second thought, they might also be a bit hard to read.
    
        As those methods are only called for object definition we could trade
some performance 
        for readability.

    objectTypes.js/ObjectType#165:
        As I don't find a nicer solution for 

            byteLength = ((byteLength - 1) | (maxReferenceLength - 1)) + 1;

        that is as short as this one, we can of course just leave it as is, a
comment what's going 
        on here would still be nice.

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
File lib/typedObjects/objectTypes.js (right):

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/objectTypes.js:51: [this.firstFree.targetBufferIndex,
this.firstFree.targetByteOffset];
Do we gain anything from [a, b] = [c, d] instead of using a = c; b = d; ? 
Otherwise this will lead to the creation of a temporary array (as far as I am
aware of) and therefore pollute the heap.

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/objectTypes.js:165: byteLength = ((byteLength - 1) |
(maxReferenceLength - 1)) + 1;
This only works correctly if either 'byteLength' or 'maxReferenceLength' is a
power-of-two. Have you considered this or better: can this become a problem?

Considering that we only use native primitive buffer views, this should not
become a problem. (Or am I missing something?)

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
File lib/typedObjects/utils.js (right):

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/utils.js:24: exports.log2 = function log2(/**Integer*/ num)
/**Integer*/
I'm not really sure is this is the right name for this method. 'log2' implies
that it works on and returns floating point values (like Math.log). Maybe we
should add something to indicate that this works with integers only (like
'ilog2'). 

Also: this method will run into an endless loop for negative values, which will
not occur when called from the creator methods but as it is exported it may be a
good idea to capture that case and throw an error.

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/utils.js:37: exports.ceilLog2 = function ceilLog2(/**Integer*/
num) /**Integer*/
See above for naming issues, maybe just call it 'nextPowerOf2'?

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/test...
File test/tests/typedObjects.js (right):

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/test...
test/tests/typedObjects.js:33: equal(ceilLog2(13), 16, "ceilLog2(16)");
I think this should read "ceilLog2(13)" instead of "ceilLog2(16)".

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/test...
test/tests/typedObjects.js:44: equal(log2(13), 3, "log2(16)");
I think this should read "log2(13)" instead of "log2(16)".

[Wladimir Palant, 2014-04-22 15:25:05]

> Another issue I see is that we should be aware of the (memory) overhead per
TypedObject buffer 
> and use buffers as large as possible to not undo the benefits of this
aproach.

That's why I made sure bufferSize can be passed in explicitly. The same buffer
size applied to all classes might either waste buffer space (too few instances
of a type so the buffer is pretty empty) or create too much overhead (lots of
instances of a type and consequently lots of buffers).

> There are two code pieces that use hard-to-understand bit manipulation
tricks

Yes, ceilLog2() isn't really obvious. What I definitely don't want here is
falling back to floating point numbers even though it *seems* that floating
point precision won't become relevant in the 32-bit range. And performance
should become important here once dynamically-sized arrays are implemented.

As to rounding up to multiples of a given power of two - yes, one has to think
twice here. However, this is really the simplest way to do it.

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
File lib/typedObjects/objectTypes.js (right):

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/objectTypes.js:51: [this.firstFree.targetBufferIndex,
this.firstFree.targetByteOffset];
On 2014/04/21 17:24:12, René Jeschke wrote:
> Do we gain anything from [a, b] = [c, d] instead of using a = c; b = d; ? 

reference.bufferIndex = reference.targetBufferIndex will change what memory area
the reference points to. Consequently, reading reference.targetByteOffset will
return bogus data.

AFAIK, using the destructuring assignment like that won't actually create a
temporary array.

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/objectTypes.js:165: byteLength = ((byteLength - 1) |
(maxReferenceLength - 1)) + 1;
On 2014/04/21 17:24:12, René Jeschke wrote:
> This only works correctly if either 'byteLength' or 'maxReferenceLength' is a
> power-of-two. Have you considered this or better: can this become a problem?

No, maxReferenceLength has to be a power of two - that's a requirement for each
primitive type. Note that we round up the size of the Reference class in
calculateSize() for that very reason (not that it changes much, size of an
object reference is 8 bytes anyway).

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/objectTypes.js:181: constructor: (typeof meta.constructor ==
"function" ? meta.constructor : null)
This was a bug - each JavaScript object has a constructor property that is a
function.

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
File lib/typedObjects/utils.js (right):

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/utils.js:24: exports.log2 = function log2(/**Integer*/ num)
/**Integer*/
Changed name into ilog2 but I didn't really feel like introducing an additional
check/branch to this function.

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/utils.js:37: exports.ceilLog2 = function ceilLog2(/**Integer*/
num) /**Integer*/
On 2014/04/21 17:24:12, René Jeschke wrote:
> See above for naming issues, maybe just call it 'nextPowerOf2'?

Renamed into nextPow2

[René Jeschke, 2014-04-22 15:40:33]

LGTM

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
File lib/typedObjects/objectTypes.js (right):

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/objectTypes.js:51: [this.firstFree.targetBufferIndex,
this.firstFree.targetByteOffset];
On 2014/04/22 15:25:05, Wladimir Palant wrote:
> On 2014/04/21 17:24:12, René Jeschke wrote:
> > Do we gain anything from [a, b] = [c, d] instead of using a = c; b = d; ? 
> 
> reference.bufferIndex = reference.targetBufferIndex will change what memory
area
> the reference points to. Consequently, reading reference.targetByteOffset will
> return bogus data.
> 
> AFAIK, using the destructuring assignment like that won't actually create a
> temporary array.

Right, using temporaries also will look ugly. So: ok.

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/objectTypes.js:165: byteLength = ((byteLength - 1) |
(maxReferenceLength - 1)) + 1;
On 2014/04/22 15:25:05, Wladimir Palant wrote:
> On 2014/04/21 17:24:12, René Jeschke wrote:
> > This only works correctly if either 'byteLength' or 'maxReferenceLength' is
a
> > power-of-two. Have you considered this or better: can this become a problem?
> 
> No, maxReferenceLength has to be a power of two - that's a requirement for
each
> primitive type. Note that we round up the size of the Reference class in
> calculateSize() for that very reason (not that it changes much, size of an
> object reference is 8 bytes anyway).

Okay then.

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/objectTypes.js:181: constructor: (typeof meta.constructor ==
"function" ? meta.constructor : null)
On 2014/04/22 15:25:05, Wladimir Palant wrote:
> This was a bug - each JavaScript object has a constructor property that is a
> function.

Didn't know that, now I do. Finally a good use case for 'hasOwnProperty'.

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
File lib/typedObjects/utils.js (right):

http://codereview.adblockplus.org/4827963358969856/diff/5629499534213120/lib/...
lib/typedObjects/utils.js:24: exports.log2 = function log2(/**Integer*/ num)
/**Integer*/
On 2014/04/22 15:25:05, Wladimir Palant wrote:
> Changed name into ilog2 but I didn't really feel like introducing an
additional
> check/branch to this function.

Alright then.