You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
to avoid all superfluous qubit list copying (even though it's only a stack copy). This gives a (mostly seamless) performance benefit in few-qubit settings (about 30% below 8 qubits). Note the name SmallView is tentative and chosen to not change line lengths; clearer choices include SmallListView, ConstSmallList, ListView, ConstList, etc.
@otbrown How do you feel about this? It's an important optimisation if we really cared about restoring v3 few-qubit performance, but I am not sure if it outweighs the reader confusion. It's main utility might be merely to assuage future readers who say "why all these superfluous SmallList copies?" - but what a big penalty to readability! :P AI assures me this is a sufficiently conventional approach in HPC applications but I'm moderatelty unconvinced. What do you think? I could take or leave.
I think it would be shame to leave performance on the table for want of a few characters. The hard part of these optimisations (that we might have wanted to avoid given the relatively small improvement) is in finding what needs to change -- you've done the hard part, so we may as well reap the benefit!
To be honest, I would just use const SmallList& -- aliasing only creates another layer of indirection for the reader to work through, and the only sensible choices are longer (which I believe would be smallListView = smallList& or maybeconstSmallListView = const smallList&), not shorter. The only good reason to alias it here is if we believe the type needs to change somewhat frequently and we want to be able to adjust it in one location.
const T& is indeed very very common in HPC codes to avoid unnecessary copies/allocations, which as you've seen can be a substantial overhead in some scenarios. I realise you're not a const enthusiast, but it can be a useful defensive coding practise as well as helping the compiler to do better when paired with pass-by-reference! 😛
I did find myself asking just now why const can't just imply pass-by-ref, and apparently for very small data (i.e. one int), pass-by-ref can actually incur a performance penalty because of the added indirection.
Agreed that it's a huge shame to cause more indirection, but the visual clutter of const SmallList& also damages readability. I do see the alaising of bah = const T& in some open source works, like here, but there the name is more explicit.
What about...
renaming SmallList to List64
renaming SmallView to ConstList64
The relationship is now very explicit; ConstList64 looks just like a list you cannot modify. And indeed that is all the reader should care about - the fact that it is also a reference is only because of the copy optimisation and irrelevant to function semantics.
(Btw I am also open to alternative names to SmallList / List64, such as IntList, etc. The suffix View was terrible in hindsight, since it invites a misinterpretation of a distinct type/object, and it's astonishing that a copy ctor back to a SmallList exists)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here, we alias
to avoid all superfluous qubit list copying (even though it's only a stack copy). This gives a (mostly seamless) performance benefit in few-qubit settings (about 30% below 8 qubits). Note the name
SmallViewis tentative and chosen to not change line lengths; clearer choices includeSmallListView,ConstSmallList,ListView,ConstList, etc.@otbrown How do you feel about this? It's an important optimisation if we really cared about restoring v3 few-qubit performance, but I am not sure if it outweighs the reader confusion. It's main utility might be merely to assuage future readers who say "why all these superfluous
SmallListcopies?" - but what a big penalty to readability! :P AI assures me this is a sufficiently conventional approach in HPC applications but I'm moderatelty unconvinced. What do you think? I could take or leave.