(#3379) - prefer readAsArrayBuffer to BinaryString

[nolanlawson]

I have no evidence that this produces better
performance, but my intuition tells me so.

It just seems silly to convert a Blob to a binary string
just for the purposes of MD5 hashing. Presumably
ArrayBuffers are better at memory management.

[nolanlawson]

Note that spark-md5 supports binary strings and array buffers, but not blobs.

[nolanlawson]

Neat, looks like I found a bug in spark-md5.

[nolanlawson]

Welp, it'll take me awhile to debug spark-md5. Adding WIP to the title.

[nolanlawson]

Ah jeez, it's a bug in our sliceShim. Got a fix now.

[nolanlawson]

^ this was the sliceShim bug. if you pass in 0 for begin and non-zero for end, then it will just call slice(), which is wrong.

[nolanlawson]

OK, this commit should be good now.

[daleharvey]

We should have a performance test that indicates this is some type of improvement, I ran the performance test and it ddint indicate any improvement, nor would I expect it since we dont have any attachment tests

[nolanlawson]

To me it's obvious for a few reasons:

1. In IE it doesn't even have readAsBinayString, so it has to readAsArrayBuffer and then convert it
2. ArrayBuffers are more performant than huge strings for binary data, right?

Although I realize the onus is on me to provide a performance test justifying this. I'll try to get one soon.

[daleharvey]

Yeh, I agree its obviously better, but its similiar to previous improvements we need to get the infrastructure in place not as much for this patch, but for future improvements to be measurable.

I am focussed on your android patch right now, once I have done that I will make another go at the improvements I plan for the performance suite

[nolanlawson]

Added a basic attachment perf test. It actually shows that the arraybuffer method is slightly slower:

• Chrome 40: 3595ms -> 3677ms
• Firefox 37: 5855ms -> 5890ms

These might be flukes, though. I should probably add more iterations and try to get a finer granularity.

Also, if the memory usage is improved, these numbers won't capture that.

[daleharvey]

Just a jshint failure here, I would be fine merging this to get the code fix and at least the performance tests in there, +1 when green

[nolanlawson]

OK, fair enough! jshint should be fixed now.

[daleharvey]

Failing in node

[nolanlawson]

Oh God, if I want this canvas stuff to work in Node, we're going to need the canvas package, which has all these dependencies.

I dunno. Maybe I should just check a large image into source control and load that?

[nolanlawson]

or just make a bigass text file

[nolanlawson]

OK, I think we have a pretty decent test now. Just directly makes some Buffers/Blobs of arbitrary size. Makes more sense than my fancy canvas stuff.

[daleharvey]

I like this test much better, a lot simpler

[daleharvey]

Merged in 98a1623