Remove move from Tile. #4749
Remove move from Tile. #4749
Conversation
KiterLuc
force-pushed
the
lr/no-tile-move
branch
from
05cf973 to
ba1cfb5
Compare
There was a problem hiding this comment.
How would this help with the memory tracking effort? The lack of move constructors has caused problems in places like #4682 (comment).
At the very least we could delete only move assignment.
| @@ -122,7 +122,7 @@ class GenericTileIO { | |||
| * @param encryption_key The encryption key to use. | |||
| * @return Status, Tile with the data. | |||
| */ | |||
| static Tile load( | |||
| static shared_ptr<Tile> load( | |||
There was a problem hiding this comment.
How about we return tdb_unique_ptr? It can avoid the overhead of shared_ptr if we don't need it, and can be converted to a shared_ptr if we need it.
There was a problem hiding this comment.
That's not a bad suggestion, but the generic tiles get passed around in our codebase and we would need extra conversions to make this work. Since a generic tile does storage access, I doubt there will be much performance impact here so let's keep it a shared pointer for now. More, at some point in the near future, we are going to fully get rid of the tile class so we can address this at that time.
|
This pull request has been linked to Shortcut Story #41591: Remove move constructor and move assign constructor from Tile and all classes using it.. |
This will help the memory tracking effort by removing all move constructors/assign operator for Tile, ResultTile, etc. --- TYPE: NO_HISTORY DESC: Remove move from Tile.
KiterLuc
force-pushed
the
lr/no-tile-move
branch
from
ba1cfb5 to
66e1d87
Compare
|
@teo-tsirpanis The problem in #4682 is a different one, and we'll solve that one independently. Here, this is actually also a good change for other reasons... There is no reason we should be moving tiles around and in some cases, we were using the move constructor when we didn't think we were. This change will prevent us from doing that again. |
There was a problem hiding this comment.
+1 on this change. I spent some time yesterday reviewing things and then checking performance and am not finding a significant change before and after this commit.
I'll spend today getting my Tile PR rebased onto this branch and then update the ResultTile instrumentation.
This will help the memory tracking effort by removing all move constructors/assign operator for Tile, ResultTile, etc.
[SC-41591]
TYPE: NO_HISTORY
DESC: Remove move from Tile.