vigna / fastutil

fastutil extends the Java™ Collections Framework by providing type-specific maps, sets, lists and queues.
Apache License 2.0
1.78k stars 196 forks source link

Rename computeIfAbsentPartial() to computeIfAbsent() #208

Closed vigna closed 3 years ago

vigna commented 3 years ago

In line with the current developments, it seems sensible to let lambdas in computeIfAbsent() to use a full type-specific fastutil function rather than the approximation provided by JDK_PRIMITIVE_FUNCTION. The infrastructure is already in place, as functions extends the associated JDK_PRIMITIVE_FUNCTION, so no ambiguity can arise. One just needs to rename computeIfAbsentPartial() to computeIfAbsent(), leaving behind a deprecated delegating stub for backward compatibility.

The only objection is that the code for a fastutil function will check for containsKey(), which for a lambda will always return true. Since it is a totally predictable test, however, the impact should be negligible (≈2ns on current hardware). The JIT might even inline it and eliminate the code.

In exchange, we get much less boxing and unboxing when using lambdas.

@incaseoftrouble , you implemented all this stuff. Your thoughts?

incaseoftrouble commented 3 years ago

Hm, no strong opinion.

Basically the issue was that "regular" computeIfAbsent does not add an association if the returned key is null (which doesn't work if you have primitive functions). So the question is whether we want to treat defaultReturnValue as null (i.e. if computed value == drv then don't add) or always add the computed value. If I recall correctly, the suffix is merely intended to clarify the behaviour.

vigna commented 3 years ago

No, all variants check for the default return value of the map, and possibly use containsKey() on the map (not on the function). That part of the code is exactly identical in all cases.

The difference is in how the remapping function is handled. The standard version (no suffix) just remaps blindly. The Nullable version uses a function that can return null, and uses null to remove the key. The partial version checks containsKey() on the remapping function (now that I think about it, in fact it could do the same as the rest of the code—first check for the default return value, and then possibly containsKey()).

My point is simply that presently if one uses the standard version with a lambda, there are many cases in which there will be some boxing or unboxing, and / or widening / narrowing casts. If we rename the partial version, this is not going to happen anymore.

vigna commented 3 years ago

In the same vein, I just noticed looking at the code for #162 that we have, say, Object2CharMap.mergeChar(). But merge() has a key and a value argument, so there is no ambiguity in calling the type-specific method merge(). Am I missing something?

incaseoftrouble commented 3 years ago

The difference is in how the remapping function is handled. The standard version (no suffix) just remaps blindly. The Nullable version uses a function that can return null, and uses null to remove the key. The partial version checks containsKey() on the remapping function (now that I think about it, in fact it could do the same as the rest of the code—first check for the default return value, and then possibly containsKey()).

Ah true, or that way. I didn't look at the code. Well I guess then it makes sense.