Avoid redundant checks of the key returned by ets:lookup in pg.erl

[RobinMorisset]

Part of pg.erl in the stdlib has the following pattern:

[{Key, Value1, Value2}] = ets:lookup(Table, Key)

Because Key appears on the left side, and is already bound, it results in an equality check between the Key that is returned by lookup and the one that was passed to lookup. This is pointless since they are guaranteed to match.

I verified on a small example, and this results in 3 bytecode instructions and 19 (!) x86 instructions, all unnecessary. It can be trivially avoided by using an unbound variable such as _Key:

[{_Key, Value1, Value2}] = ets:lookup(Table, Key)

I don't expect the performance win to be significant, but it is so simple that I think it worthwhile to land.

[github-actions[bot]]

CT Test Results

       2 files       65 suites   1h 1m 16s :stopwatch: 1 364 tests 1 225 :heavy_check_mark: 139 :zzz: 0 :x: 1 544 runs  1 363 :heavy_check_mark: 181 :zzz: 0 :x:

Results for commit 1dfaa948.

:recycle: This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[max-au]

it results in an equality check between the Key that is returned by lookup and the one that was passed to lookup. This is pointless since they are guaranteed to match

pg used that as a runtime assertion (mostly for "just in case").

You can probably use erlperf to collect some benchmarking data. I tried this test:

./erlperf '[pg:join(group, self()) || _ <- lists:seq(1, 100)], [pg:leave(group, self()) || _<-lists:seq(1, 100)].' --init 'pg:start(pg).'

But likely the difference is too small to notice, even using a benchmark.

[RobinMorisset]

I agree that the difference is likely to be too small to measure.

But I don't understand what value the runtime assertion brings. I verified, and the ETS table is not of type ordered_set. So lookup does exactly the same kind of "does it match" check that the assertion does right after. If we cannot trust the runtime to do this correctly in this simple instance, I don't see how we could reliably run anything.

[bjorng]

I agree that there is unlikely to be a noticeable performance improvements, at least if the group identifiers are terms that fit in a word, such as atoms or small integers, because in that case only the first few instructions of the 19 instruction sequence need to be executed.

However, I like the reduction in code size, and while I also like assertions, I find it unlikely that this particular assertion will catch any bugs. Therefore, I approve this pull request and will add it to our daily builds.

[bjorng]

Thanks!