feat: change DataView Grouping compileAccumulatorLoop for CSP safe

[JesperJakobsenCIM]

No description provided.

[ghiscoding]

Apart from some small Linting issues, the PR looks quite simple. Have you check logged perf? Just asking again in case there's no difference in perf, if so only 1 approach is enough. Remember that we kept the other separate because there was a slight perf decrease

[JesperJakobsenCIM]

50 rows

Small test, first one is with CSP and second test is without CSP (old) method

By general the new method actually seems faster, surprisingly me a little and its even with quite a margin of performance

This is old method first and new method next with 5000 rows instead of 50

Edit:

this is with for loop change and 50.000 rows

[ghiscoding]

The diff seems marginal, do you think we should keep both? I would probably vote for merging them and keeping only the new one, what do you think?
hmm but I wonder what's the number for 50k rows? do we see a larger diff?

Actually, taking another look at the new code, I remember seeing in the past that the for loop is the most optimized code by all browser engines and other approach like the for...of like the one you used are not the most optimized. I search on the net and found this article Measuring Performance of Different JavaScript Loop Types you can see that the plain for is always faster, I guess the .forEach is not too far of, so we could use that instead and keep the code readable while still getting better perf... so in short, perf really does matter for this project, so I think we should replace the for of code and perhaps I should take another look at the entire project for such usage, the for...in seems to be the worst to use.

Side note on an outside topic , I found why the for...in is slower compared to equivalent use of .forEach from Object.keys. It's explained in this Stack Overflow, so I think I'll revisit all the SlickGrid and SlickDataView for possible perf improvement in separate PR but I'll wait for your PR to be merged first to avoid conflicts.

The Object.keys() method returns an array of a given object's own enumerable properties, in the same order as that provided by a for...in loop (the difference being that a for-in loop enumerates properties in the prototype chain as well).

EDIT

I went ahead with PR #953, since we didn't touch the same lines, I don't expect any conflicts. However I still wish that you refactor the for...of and compare the diff with larget dataset before making a final decision if we should keep both functions or just the new function approach. Thanks

[ghiscoding]

@JesperJakobsenCIM so have you had a chance to compare if changing to a regular for loop makes a difference in performance? Do you think we should keep both function or merged into a single function? I'm waiting for your feedback before merging the PR. Thanks

[JesperJakobsenCIM]

the 50.000 example is with a for loop, and it seems to get faster since 5.000 rows (without for loop) has the same speed as 50.000 rows with for loop

[ghiscoding]

So does that mean that both functions are nearly the same speed? If so, can we keep only 1 function (the newest and CSP safe).

[JesperJakobsenCIM]

its actually marginally faster

50.000 rows test
first 5 rows is CSP Safe and last 5 rows is old function

[ghiscoding]

that is really awesome to see, faster is great 🚀

So could you refactor the code and keep only the CSP function then? I could merge it after and publish a new release, just waiting for this PR :)

[JesperJakobsenCIM]

I've removed the old method and kept new method with its name for clarity.

[ghiscoding]

This is now available in the latest release v5.7.0, thanks again for your contributions. I think this was the last piece to be fully CSP Safe, thanks a lot for all your help and guidance on the subject 🎉

Happy Holidays 🎁