I remember we had a long discussion about this, I think for http. Have we ever come to a conclusion whether this is a good idea?

The objects here are not exposed, so it's not an issue in this case.

OK. And it does improve performance? Because - at least for me - it decreases readability.

I didn't measure it specifically, but it does remove unnecessary hash table entries. It may also help if V8 sees only numeric keys being set on the object?

@fhinkel Just curious. Do you mean using a Map instead of Object.create? #7581 (comment)

For me it increases readability, but please check performance - I recall there being a regression when using .create(null) at one point.

@Florian-R Thanks, that's the discussion I was thinking of. But in this case I meant {} vs .create(null).

@benjamingr I'm not sure what regression you're referring to. These objects are created at startup, not during runtime, so the time to create them doesn't matter as much.

I understand this is not about performance, but just out of curiosity, I measured the performance difference.

console.time("{}"); for (var i = 100000; i--;) { var a = {}; } console.timeEnd("{}"); var create = Object.create; console.time("create"); for (var i = 100000; i--;) { var b = create(null); } console.timeEnd("create");

{}: 5.792ms
create: 15.221ms

Probably, calling a function is heavier than {}

Literals are always faster than instantiation and builder function calls, in JS and most languages.

Thanks. I like this much better than using arguments.

Can we check for typeof args1 == 'undefined' rather than using length? Or is undefined a valid value for args1?

I would say it's a valid value.

For me it increases readability, but please check performance - I recall there being a regression when using .create(null) at one point.

Unsure why we'd want this in an extra function

I think extracting it into a function increases readability.

The main reason is to keep the insert() size down, but it does also help a bit with readability I suppose.

...args ?

Rest args are still slow.

Great move.

Why is this a new function?

To keep function sizes down.

IMHO, there's no repetead call, you add a short flow path (and creates a mini and fake cyclomatic complexity), function is in the same script and right under the function (so you just added brackets and more names to read). Function call is slower than direct structured code (and even minimal performance gains, in this call stack queen, is important).

I would not do this separation.

unsure if changing this won't have side-effects somewhere

I grepped lib/ and ._repeat isn't used anywhere except timers.js.

I'm more concerned about user code, I do understand that it is unlikely but it has been the way it was an awfully long time in the wild.

Would you rather I leave the variable set to null since it wouldn't be used anymore and add a new property that stores the interval value?

should only be timer._handle I think

Unfortunately not, as _http_outgoing and net have a ._handle and manually enroll() pre-made objects. Node will crash since those ._handle instances are not only not Timeout instances, but they also do not have a .start() method.

And the reason this change is necessary now is that the rearming code no longer has access to the scope where the Timeout instance was created.

hmmmm ok

i would just check if (timer._repeat) iirc that is what is done elsewhere.

I don't think it's safe really since ._repeat is now being used to store the interval value, which would not work for the 0 interval.

@mscdex you can't have an interval of 0. It becomes 1.

Changed.

I don't see any tangible benefit to doing this.

We only use Number value keys.

As was previously mentioned, I didn't measure the change specifically, but it does remove unnecessary hash table entries. It may also help if V8 sees only numeric keys being set on the object? It has nothing to do with key name collisions in this case.

if ontimeout() throws will it deo-opt that function?

Which function? tryOnTimeout()? ontimeout()? The user's callback?

I don't know that throwing causes a deopt or not, but adding a try-* block certainly does cause a deopt for the containing function.

but adding a try-* block certainly does cause a deopt for the containing function.

Right, which is why it was constrained to within one separate function.

I'm curious if a throw fork a user callback de-opts ontimeout() along the way though, since that could be somewhat unpleasant.

Maybe we shouldn't worry about it? Idk the sate of benchmarking this stuff is already so bad 😐

I just wrote a little test and as far as I can tell a function (or any calling functions in the call stack) shouldn't get deopted if that function throws.

Try deopt any container function. Node.js should, IMHO, always test for nullity and use flags/optionals and never try-catch in the inner functions... This pattern is not really a requirement in functional programming and there's huge performance benefits on native calls (basically, in any platform too. try-catch system is expensive).

I'm more concerned about user code, I do understand that it is unlikely but it has been the way it was an awfully long time in the wild.

It probably doesn't matter much, but you could use len here instead of arguments.length.

Changed.

Literals are always faster than instantiation and builder function calls, in JS and most languages.

Try deopt any container function. Node.js should, IMHO, always test for nullity and use flags/optionals and never try-catch in the inner functions... This pattern is not really a requirement in functional programming and there's huge performance benefits on native calls (basically, in any platform too. try-catch system is expensive).

Great move.

IMHO, there's no repetead call, you add a short flow path (and creates a mini and fake cyclomatic complexity), function is in the same script and right under the function (so you just added brackets and more names to read). Function call is slower than direct structured code (and even minimal performance gains, in this call stack queen, is important).

I would not do this separation.

This is always better for internal workings. ALWAYS, Node community.
Avoid this, globals, function calls, function separation in minimal pieces and inner functions... and we'll always have fine code with max JS performance.