Depreciated legacy tree map #963

DavidM-D · 2022-11-15T16:31:40Z

I have the alternative of marking each individual use of legacy tree map in legacy_tree_map.rs as depreciated rather than the whole module, but since the whole source file is depreciated I figured it wasn't worth the effort.

Resolves #868

austinabell · 2022-11-15T17:59:05Z

near-sdk/src/collections/legacy_tree_map.rs

@@ -18,6 +20,7 @@ use crate::IntoStorageKey;
 /// - `above`/`below`:          O(log(N))
 /// - `range` of K elements:    O(Klog(N))
 ///
+#[deprecated(since = "4.1.0", note = "Use tree_map::TreeMap instead")]


Unfortunately, it's not that simple. Just swapping to use this type is a state-breaking change. At least the note should indicate this.

Also, the tree_map module is not exposed, so you might want to change this to say near_sdk::collections::TreeMap instead.

Optionally, this PR could introduce some migration of the old collection to the new one as a method, for example something like LegacyTreeMap::to_tree_map(&self) or something like this and then this comment can point to this to make the migration easy. This is less important, though, as I presume no one is really using this and if they absolutely want it, they can just ignore the deprecation warnings

So many mistakes in one line!

DavidM-D · 2022-11-24T18:47:15Z

An issue I'm having with this migration code is that it appears to be hitting the gas limit after successive runs. How should I go about resetting the gas limit after each run?

The gas limit also seems very low, it seems to fall over with 100 elements, is that to be expected or am I doing something stupid?

austinabell · 2022-11-25T15:25:23Z

near-sdk/src/collections/legacy_tree_map.rs

+        // We read this into the heap first so one can use the same prefix without getting into trouble
+        let elements: Vec<(K, V)> = self.into_iter().collect();
+        let mut tm = super::TreeMap::new(prefix);
+        for (k,v) in elements.iter() {


This all happens synchronously, what would be a case that this would solve a race condition that using the iterator directly in the loop wouldn't?

austinabell · 2022-11-25T15:26:40Z

near-sdk/src/collections/legacy_tree_map.rs

@@ -585,6 +585,16 @@ where
        self.save(&n);
        self.tree.pop();
    }
+
+    pub fn to_tree_map<S: IntoStorageKey>(self, prefix: S) -> super::TreeMap<K, V> {


Suggested change

pub fn to_tree_map<S: IntoStorageKey>(self, prefix: S) -> super::TreeMap<K, V> {

pub fn into_tree_map<S: IntoStorageKey>(self, prefix: S) -> super::TreeMap<K, V> {

Just to follow https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv naming conventions.

Also, would be nice to have a small comment describing this fn

austinabell · 2022-11-25T15:39:21Z

near-sdk/src/collections/legacy_tree_map.rs

+
+    pub fn to_tree_map<S: IntoStorageKey>(self, prefix: S) -> super::TreeMap<K, V> {
+        // We read this into the heap first so one can use the same prefix without getting into trouble
+        let elements: Vec<(K, V)> = self.into_iter().collect();


So I'm curious about if this should be clearing the state when doing this conversion.

Seems like if the API is (self) -> Self it would be intuitive that the storage would be cleared and the new collection would be filled with this data.

Otherwise, we could consider this pattern of not clearing and have the method be something like:

fn copy_to_tree_map(&self, ..) -> super::TreeMap...

with a better name and whatnot.

Also, the other part that might seem opinionated is if the prefix is added as a parameter here. This makes sense if we go the copy route, but if the API is for an owned self, it might be more intuitive to just remove the prefix and re-use the one from which you're clearing.

To summarize, my intuitions are that this should either:

Remove the prefix from parameters, re-use and clear the state, put state back in at the same prefix

Change this API to &self and make it clear that the elements are not being removed from storage

Clearly document this fact and make it clear to devs that they should not use the same prefix as before (seems like a footgun and also requires devs to manually clear the state of the old collection)

Another option for fun: Just create a drain iterator that clears and gives all key-value pairs back. Let the user decide if they want to clear the elements and put it into another collection or to just use the current iterator and use that to just fill another collection

This gives more freedom to migrate this state into other data structures, rather than just the collections::ltm -> collections::tm path

Seems like it might be good to get another opinion on this. @itegulov, you might be most familiar with these APIs, wdyt?

austinabell · 2022-11-25T15:41:33Z

An issue I'm having with this migration code is that it appears to be hitting the gas limit after successive runs. How should I go about resetting the gas limit after each run?

The gas limit also seems very low, it seems to fall over with 100 elements, is that to be expected or am I doing something stupid?

That sounds expected. This tree-map is very inefficient. I wonder if that's even more reason to do a drain(..) iterator for this so that the migration could be done in chunks.

DavidM-D · 2022-11-29T12:40:09Z

OK I'll remove the migration code until someone asks for it, then we can better understand their use case

austinabell · 2022-11-29T15:28:27Z

near-sdk/src/collections/legacy_tree_map.rs

@@ -18,6 +20,10 @@ use crate::IntoStorageKey;
 /// - `above`/`below`:          O(log(N))
 /// - `range` of K elements:    O(Klog(N))
 ///
+#[deprecated(
+    since = "4.1.0",
+    note = "Use near_sdk::collections::TreeMap, if you have an existing LegacyTreeMap use LegacyTreeMap::to_tree_map to migrate"


So this comment is out of date now, just remove the comment about migration and this is good

austinabell · 2022-11-29T15:54:31Z

near-sdk/src/collections/legacy_tree_map.rs

@@ -18,6 +20,7 @@ use crate::IntoStorageKey;
 /// - `above`/`below`:          O(log(N))
 /// - `range` of K elements:    O(Klog(N))
 ///
+#[deprecated(since = "4.1.0", note = "Use near_sdk::collections::TreeMap")]


noting that the since "4.1.0" is a bit unideal, but given we don't really have a plan what will be the next minor/major version, and no one uses this, it's probably fine

So yeah, how do we feel about depreciations? Are the major release only, or do we just do it whenever?

Depreciated legacy tree map

aa31dac

DavidM-D requested review from austinabell, ChaoticTempest and itegulov as code owners November 15, 2022 16:31

DavidM-D changed the title ~~Depreciated legacy tree map #868~~ Depreciated legacy tree map Nov 15, 2022

austinabell reviewed Nov 15, 2022

View reviewed changes

Added migration and prop test of it

753f4f4

Remove diagnostic print

81edc9c

austinabell reviewed Nov 25, 2022

View reviewed changes

Removed migration code until someone asks for it

99c8193

austinabell reviewed Nov 29, 2022

View reviewed changes

Removed migration guide

7dcbd6d

austinabell approved these changes Nov 29, 2022

View reviewed changes

Merge branch 'master' into depreciate-legacy-tree-map

22eb479

DavidM-D merged commit 1849287 into near:master Nov 29, 2022

frol mentioned this pull request Aug 17, 2023

chore: release v5.0.0-alpha.1 #1070

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Depreciated legacy tree map #963

Depreciated legacy tree map #963

DavidM-D commented Nov 15, 2022 •

edited

Loading

austinabell Nov 15, 2022

DavidM-D Nov 24, 2022

DavidM-D commented Nov 24, 2022

austinabell Nov 25, 2022

austinabell Nov 25, 2022

austinabell Nov 25, 2022

austinabell commented Nov 25, 2022

DavidM-D commented Nov 29, 2022

austinabell Nov 29, 2022

austinabell Nov 29, 2022

DavidM-D Nov 29, 2022

	pub fn to_tree_map<S: IntoStorageKey>(self, prefix: S) -> super::TreeMap<K, V> {
	pub fn into_tree_map<S: IntoStorageKey>(self, prefix: S) -> super::TreeMap<K, V> {

Depreciated legacy tree map #963

Depreciated legacy tree map #963

Conversation

DavidM-D commented Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidM-D commented Nov 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell commented Nov 25, 2022

DavidM-D commented Nov 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidM-D commented Nov 15, 2022 •

edited

Loading