Implement database iterator #47
Conversation
Enumeration time for cities database in release mode is around 12 seconds now. Debug is around 20.
|
I would suggest a squash and merge on this to avoid soiling the commit history |
This allows any type to easily be iterated, at the cost of losing the convenience of a foreach loop.
MaxMind.Db/Reader.cs
Outdated
| /// <summary> | ||
| /// Get an enumerator that iterates all data nodes in the database | ||
| /// </summary> | ||
| /// <param name="cacheSize">The size of the data cache. This can greatly speed enumeration at the cost of memory usage.</param> |
There was a problem hiding this comment.
This should also probably warn about the dangers of mutating the returned object given that the object is in the cache.
MaxMind.Db.Test/ReaderTest.cs
Outdated
| } | ||
|
|
||
| [Fact] | ||
| public void TestEnumerateCitiesDatabaseSpeed() |
There was a problem hiding this comment.
This test is rather slow and would likely fail if run on a slow machine. Perhaps this could be refactored a bit and moved to be with the benchmarks in MaxMind.Db.Benchmark.
MaxMind.Db.Test/ReaderTest.cs
Outdated
| { | ||
| Stopwatch timer = Stopwatch.StartNew(); | ||
| int count = 0; | ||
| using (var reader = new Reader(Path.Combine(_testDataRoot, "../../GeoLite2-Country.mmdb"), FileAccessMode.Memory)) |
There was a problem hiding this comment.
Perhaps this one could be moved to the benchmarks too. I noticed that you added this database to the repo. Although it is much smaller than the GeoLite2-City database that was added to the repo early on, I'd like to avoid adding new binary files to reduce repo bloat.
| /// A dictionary that caches up to N values in memory. Once the dictionary reaches N count, the last item in the internal list is removed. | ||
| /// New items are always added to the start of the internal list. | ||
| /// </summary> | ||
| internal class CachedDictionary<TKey, TValue> : IDictionary<TKey, TValue>, IDisposable |
There was a problem hiding this comment.
Nice! Was this based on another caching implementation or is it completely new code? I am just wondering for licensing reasons.
There was a problem hiding this comment.
Something I use in other projects, can tag it with MIT license. All code written by me in my spare time for free.
There was a problem hiding this comment.
Thanks! If you are the owner of the code, would you be willing to place it under the Apache 2 license to match the other code in the repo and for the benefit of the patent grant?
There was a problem hiding this comment.
Commented code eradicated
MaxMind.Db/CachedDictionary.cs
Outdated
| /// <summary> | ||
| /// The first node in the priority list | ||
| /// </summary> | ||
| protected LinkedListNode<KeyValuePair<TKey, TValue>> FirstNode |
MaxMind.Db/CachedDictionary.cs
Outdated
| /// <summary> | ||
| /// Current comparer | ||
| /// </summary> | ||
| public IEqualityComparer<TKey> Comparer |
There was a problem hiding this comment.
This and the following three are also not used.
MaxMind.Db/Reader.cs
Outdated
| /// </summary> | ||
| /// <param name="cacheSize">The size of the data cache. This can greatly speed enumeration at the cost of memory usage.</param> | ||
| /// <returns>Enumerator for all data nodes</returns> | ||
| public IEnumerable<Reader.ReaderIteratorNode<T>> FindAll<T>(int cacheSize = 16384) where T : class |
There was a problem hiding this comment.
A test that used something other than Dictionary<string, object> might be good. In particular, one using the TypeHolder class with the MaxMind-DB-test-decoder.mmdb database.
|
I've implemented most of these changes. I've moved the code to the benchmark project, but it is commented out for now. Not sure how useful it is at the moment, perhaps someone else can add it later if needed. |
|
Looks good. Perhaps the benchmark code could be moved to a separate PR or an issue so that we don't have commented out code lying around in the codebase. Other than that and the license question, I am happy to merge this. |
|
Ready to merge when you are. |
|
Looks great! Thanks. I am squashing it so that the binary database does not end up in the Git history. |
I would appreciate code review on this. It passes all tests.
I would love to see this in a nuget package soon.
As a side note, some of the threading tests are failing. I did not investigate that.