Improve deque, tables, and set modules #13620
Conversation
|
Those build failures don't seem related to these changes |
|
lots of good stuff in this PR. proposal 1 (simplest but not best)hashes.nim should define: proch hashIterable*[T](a: T): Hash =
for ai in a:
result = result !& hash(ai) # can later be improved via murmurhash
result = !$hand then: template hash*(a: Deque): untyped = hashIterable(a)proposal 2 (best IMO)yet another use case for proch hash*[T](a: T): Hash {.enableif: isIterable(a).} =
for ai in a:
result = result !& hash(ai) # can later be improved via murmurhash
result = !$hthis may be possible using concepts but concepts have caveats, lots of bugs, exponential complexity issues and limitations, see #12048; enableif is actually straightforward and incredibly simpler than concepts |
|
Improved the |
|
Woops, didn't mean to change that.. |
|
No Azure testing was done on this! Investigating... |
this would actually not be correct, since elements are in arbitrary order; I'm fixing hash(HashSet) in #13649 |
=> tracking here: #13653 |
hash(HashSet) which was incorrect; fix hash(OrderedTable[string, JsonNode]) which was bad
#13649
|
Again I don't think those errors are related to this PR |
|
Rebased to current devel to see if the CI will pass now |
|
@PMunch failure seems unrelated:
but also I don't have this failure in other PRs, so maybe rebase once more? |
|
ping @PMunch |
timotheecour
added
the
stale
|
ping @PMunch (and needs rebase) |
|
Is there anything apart from rebase that needs doing? |
|
well i already gave LGTM, but it also depends on #13620 (comment), let's see what CI gives after rebase |
The deque module has now gotten `high`, `toDeque`, and `hash` along with improved documentation and improved examples. All three modules now have their own default initial capacity, and these are intdefines, so they can be changed by developers on compile-time for performance tuning or running on low-end hardware.
|
Please rebase one more time, the CIs are finally green once again for devel. |
|
Done, everything should be OK now |
|
I've finally understood the implications of these changes and I don't like them anymore. Sorry. For performance tuning, use |
|
Probably we can remove some compiles call in lib? ref remove some when compiles calls from tables/deques: |
Care to elaborate? I've submitted and maintained this PR for quite a while now so it's a bit disheartening to see it dropped with only a vague sentence as to why. Which implications? Surely you don't have anything against that I implemented |
|
Hmm, let me try again: The If the stdlib uses too much memory for your system because of these default capacities the stdlib should make use of the capacity parameters that already exist. Alternatively, you could change the default global value but please not with yet another |
|
|
||
| template checkIfInitialized(deq: typed) = | ||
| when compiles(defaultInitialSize): | ||
| when declared(nimDequeDefaultInitialCapacity): |
There was a problem hiding this comment.
Pre-exists. But why this line is needed? It seems unnecessary. See also nim-works/nimskull#161 (comment)
why? it doesnt introduce new dialect. |
|
Thank you for reopening this. The issue I was running into wasn't with the stdlib, but rather with a 3rd party library. It is quite common in Nim to see people creating a new container without passing any size, or just not calling |
|
Small applications, if it turns out they need a different setting, can trivially be changed (since they're small), meaning that the feature is not that useful in these cases. Anything that uses a library (ie that solves a more complex problem and smartly reuses existing code instead of reinventing) cannot productively use -d flags at all: it's a global setting and there's no way to set one value for library A and another for library B - by necessity, they need the libraries to etiher expose library-specific flags or dynamic switches, meaning that the right place to introduce a fix is in that library. Another way to put it is that -d flags are nice toys for micro-benchmarks, but not much else, and come at a cost. The long-term benefit is limited - the long-term maintenance overhead however is not - once it's there, it's hard to remove. |
|
and how does this particular set of |
|
@arnetheduck, while I don't disagree with you in general this is one specific instance where it actually makes sense to use these flags. Any code, small or large which actually cares about what the initial size of these collections are can (and currently do) simply set the initial size in the constructor. These new flags have absolutely no interest to library creators. What these flags do offer, and where I start disagreeing with your sentiment, is the ability for an end-user to tweak the program they're writing for all those libraries that don't care what the initial size of their collections are. The only reason to change these values are for severely memory constrained targets (and perhaps extremely memory rich targets). All they do is change the initial capacity of containers that didn't bother to specify that they wanted a particular initial capacity. There are no dialects, no change of logic, nothing to really maintain. There's only a small flag being passed on to the memory allocator. |
We can eventually remove some of |
This doesn't really answer the question at all though. Why is fewer |
because they (tend to) represent a very narrow use case (often it's "workaround for poorly designed library X"), while increasing test requirements exponentially - further, they have to be maintained indefinitely (once added, code starts relying on them) and this prevents more useful modifications being done to to standard library in general - std lib sitting at the root of every dependency chain out there multiplies this cost (as opposed to changes in "leaf" code) |
|
Still no real argument against these particular switches. |
An argument against switches in general is an argurment against any particular switch. And especially it's an argument against adding yet more switches. Sounds like basic logic to me. |
Every switch needs to be documented and communicated. What's wrong with the idea to tweak the defaults for version 2.0 so that it works better for embedded? In my naive world, if it works on embedded it works well elsewhere too. |
The problem here is that for embedded I'd probably want to set them to 1. Never allocate more than what's absolutely necessary. But these would be silly defaults for non-embedded as it would quickly need to reallocate for the first couple adds until it got up to a reasonable size (after all it's not set to 0 for a reason). Different platforms have different tradeoffs, and for that reason should be treated differently. I get where the arguments against these switches come from, but in this particular case they don't make much sense. There's nothing more to test, hashing order isn't supposed to be deterministic anyways, so code that depends on it is already broken. If you actually care about the default size of a container simply check the corresponding constant in your code, or just set the initial size to something useful. These are implementation details, so no code should depend on them. And worst case scenario if they do depend on them it simply wouldn't work for certain values. |
|
This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions. |
|
This pull request has been marked as stale and closed due to inactivity after 395 days. |
The deque module has now gotten
high,toDeque, andhashalong withimproved documentation and improved examples.
All three modules now have their own default initial capacity, and these
are intdefines, so they can be changed by developers on compile-time for
performance tuning or running on low-end hardware.