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

hoon: make sane memory-efficient for large atoms #6381

Merged
merged 3 commits into from Mar 13, 2023

Conversation

johnhyde
Copy link
Contributor

@johnhyde johnhyde commented Mar 9, 2023

Description

Resolves #6380.

I updated the section of sane which checks for %t validity to iterate through bytes with an incrementing cursor. This greatly reduces memory load.

Copy link
Member

@joemfb joemfb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good catch! +rsh loops are convenient to write, but horribly inefficient.

I have a style nit:

$(i +(i))
== ==
==
$(inx +(inx))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation here is not idiomatic, it should look something like:

=+  tef=(teff cur)
?&  ?|  =(1 tef)
        =+  i=1
        |-  ^-  ?
        ?|  =(i tef)
            ?&  (gte (cut 3 [(add i inx) 1] b) 128)
                $(i +(i))
    ==  ==  ==
  $(inx +(inx))
==

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks. I was getting mixed up with the indent style for ?- and ?+. But shouldn't $(inx +(inx)) be at the same indentation level as the == == ==?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right.

@johnhyde
Copy link
Contributor Author

Can we squash these commits into a single nice message on merge, or do I need to try to rewrite history to abide by the commit message guidelines?

Copy link
Member

@joemfb joemfb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRs are usually merged instead of squashed, so you can use rebase to squash these commits down to one.

this will do

@jalehman
Copy link
Member

We normally don't squash commits, so this is good to go with no further changes needed.

@jalehman jalehman merged commit 8e3ca9b into urbit:develop Mar 13, 2023
1 check passed
@johnhyde johnhyde deleted the i/6380/fix-sane-memory-usage branch March 14, 2023 19:43
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.

Performance Issue: sane uses too much memory for large atoms
3 participants