Performance tweaks - Githubissues

james-camilleri commented 5 years ago

Proposed performance tweaks for deep gets, discussed in #41. Function falls back to previous linear search if key not found and strict mode is disabled. If strict mode is enabled (through global config) the function will throw an error.

james-camilleri commented 5 years ago

So I had actually implemented the single sort tweak for the non-strict mode, but the first time I had implemented it I hadn't realised that the function was being called recursively. While it's way faster than sorting for every single lookup, it's not quite a single sort of the whole flattened map. Since with the last commit the get-token only results in a linear search if the key cannot be found at all, as a sort of fallback, I wasn't sure if it was worth pre-sorting the map. I suppose it would depend on the real world usage - if a high perecentage of get-token() calls are made with non-existent keys, the pre-sorting is worth it, but if it's only a couple of missed hits this way might actually be faster. Not entirely sure how I'd go about testing that.

I can write the documentation, where would you suggest it goes? As a sassdoc comment for the configuration flag or should it part of the more general explanation?

mirisuzanne commented 5 years ago

if a high perecentage of get-token() calls are made with non-existent keys, the pre-sorting is worth it, but if it's only a couple of missed hits this way might actually be faster. Not entirely sure how I'd go about testing that.

Got it - let's leave it out for now. (The map flattening is a similar trade-off, but for usage of ->)

I can write the documentation, where would you suggest it goes? As a sassdoc comment for the configuration flag or should it part of the more general explanation?

I think it works as a comment on the flag. We might consider defaulting it true when we do a major release, and could add summary docs at that point.

james-camilleri commented 5 years ago

So I just thought about something regarding the error on missing keys. Would it make more sense to return null, as the standard map-get() does? Or should it be another option? Or is failing outright more sensible? It occurred to me where there might be cases that a user would want to carry out an alternate action if a key doesn't exist, and throwing an error doesn't allow for that.

mirisuzanne commented 5 years ago

Since we're adding a toggle anyway - would it be difficult for that toggle to allow several values?

the legacy silent fail
return null
return error

Is null ever useful for internal map references, or only for explicit token-get() calls?

james-camilleri commented 5 years ago

I wasn't sure what you meant by that last bit, but now that I've started coding it I think I know what you're getting at. We probably shouldn't return null for internal references, it's akin to a silent fail. Or rather we should ensure that the null carries on up the chain. So if I were to request a get-token() for a key that maps to the following string: '#ref1 #ref2->ref3 10px', and 'ref3' doesn't exist, should it return 'value1 10px' or null? I'm leaning towards the latter (which is how it currently works), because it will be impossible to know if bits of your references are wrong because of how they'll get converted into strings.

Also the tests are currently a bit slapdash, I'd like to beef them up a bit more to handle these edge cases. Should they be sectioned off into their own region for tests specifically related to the error handling? We could avoid that unsightly toggling of the error modes too.

mirisuzanne commented 5 years ago

Yeah, that's the issue I was thinking about. I think you're right about the best way to handle it if we're using null - but I'm not entirely sure that works as a universal solution: we may still want a toggle of some kind. An explicit error would be more helpful in debugging where things went wrong. I can see places I'd want silent failure like that (e.g. get-token('optional') or $fallback) - but also a lot of times that I would rather have the explicit error behavior.

I'm pretty flexible on where the tests live: some balance of:

roughly match the code structure itself
break things out into sensible chunks

mirisuzanne commented 5 years ago

@jcamilleri13 What's the status of this? I got distracted with some traveling, but I'd love to get it merged in.

james-camilleri commented 5 years ago

Hey, sorry about that, literally just walked off the plane too. So the code is functioning with a 3-way toggle (silent, null, and error). Should have committed all the changes to the issue branch, but I'll double check them tomorrow. Regarding the tests, if we're good with a three way switch I'm going to break them out into a separate file because they feel a bit ham-fisted in their current placement.

mirisuzanne commented 5 years ago

@jcamilleri13 That all work for me, and sounds great!

No rush, we're all working on Open-Source Time, just didn't want to lose track of it. :)