Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Running time notes #172

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

milesfrain
Copy link
Contributor

It would be nice to be upfront about the runtime benefit of snoc, rather than having users confirm this for themselves by looking in Arrays.js.

@garyb
Copy link
Member

garyb commented May 22, 2020

snoc is O(n) also - all the array-modifying functions are at least O(n) since they need to copy the whole array first.

@hdgarrood
Copy link
Contributor

all the array-modifying functions are at least O(n) since they need to copy the whole array first.

Perhaps it would be best to expand and clarify the note at the top of the module instead; this is useful information and it's left implicit by that note right now.

@milesfrain
Copy link
Contributor Author

My mistake. I assumed slice returned a new reference to the same underlying array (rather than copying), although I now see how that would be undesirable in lots of situations.

Could an optimizer figure out that this is our intention, and avoid copying the array upon push?

@hdgarrood
Copy link
Contributor

I highly doubt it; I can’t see that being safe unless it’s handled by the JS VM.

@garyb
Copy link
Member

garyb commented May 22, 2020

Yeah, I'm not sure why it's implemented with slice rather than it being x.concat([y]), I assume it was benchmarked and the push version was faster.

The only way we could safely avoid copying arrays is with linear types, so we know they're not reused, etc.

@milesfrain
Copy link
Contributor Author

Perhaps it would be best to expand and clarify the note at the top of the module instead; this is useful information and it's left implicit by that note right now.

Applied that change in the latest commit.

Now what should the runtime notes be for the other functions? Should we either:

  • Remove all additional O(n) notes
  • Make sure every function has a runtime note

For the latter, which note format do you prefer? I see these three varieties in the docs:

  • Note, the running time of this function is O(n).

  • Running time: O(n).

  • Running time: O(n) where n is the length of the array

@hdgarrood
Copy link
Contributor

I would prefer the third. I think we should explicitly say what n is whenever we are using big O notation, ideally.

src/Data/Array.purs Outdated Show resolved Hide resolved
@hdgarrood
Copy link
Contributor

I think most functions should have complexity noted, although not all. For instance, it doesn't really make sense to give singleton one, because there's no sensible way of defining the size of the input. Also, we can't give one for fromFoldable, because it depends on the Foldable instance in use.

@milesfrain
Copy link
Contributor Author

I think most functions should have complexity noted

Added some more running time notes. Wasn't sure about some of these though (updateAtIndices, sort, etc.), so just left those blank.

Also seems like filterA could be O(n^2), depending on the predicate, so just noted "At least O(n)".

@milesfrain milesfrain changed the title Running time note for snoc Running time notes May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants