Improve DOMTokenList performance #3
Conversation
There was a problem hiding this comment.
OK, I really appreciate the fact you are that interested in this project, even if I told you it wasn't the right time to talk about performance yet ... but this PR went too far, example:
I'm sorry, I'm against code pollution for no reason, and replacing a native array join in 2017 is no reason to me, same goes for includes, which also guards against other things too.
Just to re-clarify again, this project aim is not to be the fastest, I actually don't care at all now, it gave me the ability to facade the whole DOM in NativeScript and work as expected.
Here the key: working, and easy to maintain, to me are way more valuable things than for loops instead of native functionalities and I also believe micro benchmarks are the death of the platform.
Apologies if this comment does not welcome your interests about this projects, but either you drop all native replacements or I'm afraid I won't put myself in the situation to maintain unnecessary extra LOC for some millisecond I don't even believe is needed, at this stage (or ever, really, go compiled modules if performances has to be that extreme)
Hope you'll understand this comment.
Best Regards
| this.push(token); | ||
| } else { | ||
| main: for (let i = 0; i < adding; i++) { | ||
| for (let j = 0; j < current; j++) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
I cannot see anything in that link but O(n^2) wasn't a question, rather a statement. The algorithm used in here is not even a performance oriented one so I'm afraid this change won't land in this project
| if (i < 0) this.add(newToken); | ||
| else this[i] = newToken; | ||
| afterChanges(this); | ||
| for (let i = 0, n = this.length; i < n; i++) { |
There was a problem hiding this comment.
are you avoiding indexOf here? If that's the case ... why?
There was a problem hiding this comment.
There was a problem hiding this comment.
I cannot see anything in that link but regardless indexOf is one word to maintain VS few lines so this change won't land in this project
src/DOMTokenList.js
Outdated
| return afterChanges(this); | ||
| } | ||
| } | ||
| this.add(newToken); |
There was a problem hiding this comment.
or maybe afterChanges was triggered twice? curious to know more about this one
| @@ -23,7 +23,10 @@ module.exports = class DOMTokenList extends Array { | |||
| } | |||
|
|
|||
| contains(token) { | |||
| return this.includes(token); | |||
| for (let i = 0, n = this.length; i < n; i++) { | |||
There was a problem hiding this comment.
and here you are avoiding native includes ... I'm not too fund of augmenting complexity for no reason when the platform provides default alternatives
There was a problem hiding this comment.
There was a problem hiding this comment.
I cannot see anything in that link but regardless includes does more than your code so this change won't land in this project
There was a problem hiding this comment.
That's a glaring oversight. I'll be sure to refer to the reference implementation. Thanks for pointing it out!
|
I understand entirely @WebReflection. I see your point and I welcome your approach, despite not fully agreeing with it. (I'll leave reference replies to the relative comments, but they are by no means a way to dispute your ideas or work. Despite the striking difference: a) I'm not entirely sure how they translate in real-world usage; b) I didn't test them in Node. A contrasting observation might be that the difference could be more significant on mobile.) |
ghost
closed this
I've been around for too long to smell 10 years ago useless optimizations in here ... the JIT era went way beyond this and modern engines should be able to optimize better than JS itself. I'm really not coding thinking that I cannot Again, I appreciate your interest to this project but if it's crazy-faster-super-performance you are looking for, you're better off with any of the alternative out there, really: this project is not about crazy-fast performance ... this project is about bringing, with ease to read and contribute, a basic HTML env to any non HTML environment. On nodeJS I would not use this, unless I've pre-created documents per page ... and there indeed the serialization via |
|
P.S. you can fork this project and bring in all the micro-benchmarks you want. Maybe one day all will be merged back together 😃 |
I was hoping to borrow some classes here and there — while I was there I thought why not.