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

Lines take too long to print #625

Open
ohAitch opened this issue Dec 3, 2015 · 17 comments
Open

Lines take too long to print #625

ohAitch opened this issue Dec 3, 2015 · 17 comments

Comments

@ohAitch
Copy link
Contributor

ohAitch commented Dec 3, 2015

This bug was introduced by a4abd16, is the root cause of #507, and of me running into #592.

Somewhere in the printing system, as ~+ hint is being thrown, and the hint is not being honored. Doing so causes a memory leak; not doing so causes unreasonably slow performance for

  • notifications of large numbers of synced files
  • dojo expressions such as ut(sample contains the type of ++twig several times over).
  • some stack traces(though those are restricted in size)
  • talk backlog(likewise, somewhat)
@chc4
Copy link
Contributor

chc4 commented Dec 3, 2015

Isn't this the more generic "need a high-level memoization cache" problem? I remember people saying a few other things are hit by not honoring siglus on the top road, and was one of the things that Curtis worried about for proper implementation of an ++mint:ut cache for %gall.

It might be worth mentioning that large stacktraces take an extremely long time to print for me, and only appear to start buffering after everything is finished. That is, while :talk fetches the chat backlog it doesn't print while it's getting the rest, and only starts the long printout after fully synced. I'm not sure if that's because :talk returns the lines all at once, though?

@galenwp
Copy link
Contributor

galenwp commented Feb 8, 2017

I'm uncertain if this is a 'bug' or a feature request. @ohAitch can you clarify?

@ohAitch
Copy link
Contributor Author

ohAitch commented Feb 8, 2017

It is a bug, introduced by a4abd16. If you revert it the problem goes away, but there's a permanent store leak that it did in fact fix.

@jtobin
Copy link
Contributor

jtobin commented Jun 19, 2019

Closing as I'm not sure this is still relevant, and I'm biasing towards a scorched-earth approach on older issues.

@jtobin jtobin closed this as completed Jun 19, 2019
@ohAitch
Copy link
Contributor Author

ohAitch commented Jun 19, 2019

Does the linked commit still cleanly revert? If it does, is there a noticeable difference in the responsiveness of running ut in dojo?

(I vaguely remember something about memoization being restructured, but "not sure this is still relevant" seems like the opposite of having a reason to close a ticket. Bug bankrupcies are not beneficial, though I suppose if this is effectively a MAYFIX classification I shouldn't take offense.)

@jtobin
Copy link
Contributor

jtobin commented Jun 19, 2019

"not sure this is still relevant" seems like the opposite of having a reason to close a ticket. Bug bankrupcies are not beneficial, though I suppose if this is effectively a MAYFIX classification I shouldn't take offense.

Definitely don't take offence -- as you may have noticed, I've just been trawling through the issues list trying to clean up things that I don't expect can be worked on as-is. I expect collateral damage, but I appraise a particularly-actionable issues list as being worth the cost.

A longer justification for closing this one is that "Lines take too long to print" is probably no longer an accurate characterisation of a current issue; the specific problems you describe may certainly still be relevant, but should probably be repackaged/re-presented, if so.

@ohAitch
Copy link
Contributor Author

ohAitch commented Jun 19, 2019 via email

@joemfb
Copy link
Member

joemfb commented Jun 19, 2019

Slow printing is definitely among our lowest-hanging performance fruit, just waiting to be plucked. It's probably gotten worse since this was opened -- there are multiple unicode conversion passes over +tank, among other evils.

I haven't looked into ~+ on the home road; do you remember if it caused a memory leak detected by GC, or a space leak?

As far as this issue is concerned ... On the one hand, the entire printing pathway should be rethought (or perhaps burnt in a cleansing fire), on the other, this is a specific characterization of a specific problem that is likely still active (poor performance caused by disabling a useful but broken feature). Per the test of "actionable as is", I favor keeping this one open.

@jtobin
Copy link
Contributor

jtobin commented Jun 19, 2019

this is a specific characterization of a specific problem that is likely still active (poor performance caused by disabling a useful but broken feature). Per the test of "actionable as is", I favor keeping this one open.

say no more

@jtobin jtobin reopened this Jun 19, 2019
@Fang- Fang- removed the u3 label Aug 13, 2019
@tacryt-socryp
Copy link
Contributor

Ping. Performance 👀

@tacryt-socryp
Copy link
Contributor

Has this been resolved by the most recent round of Hoon changes? @joemfb @jtobin

@ohAitch
Copy link
Contributor Author

ohAitch commented Dec 18, 2020

Well, raft.c no longer exists, and noun.c is now a bytecode interpreter, so the patch certainly no longer reverts cleanly XD

@philipcmonk do you know if "the outer road doesn't have access to memoization" is still an accurate statement in the current system? I do vaguely remember seeing a PR about this.

@joemfb
Copy link
Member

joemfb commented Dec 18, 2020

Memoization is still disabled on the home road, and the memoization cache is still not promoted across roads. Enabling either is simple, having confidence in the correctness and/or righteousness of either is somewhat less so.

Note that the ~+ in question is most likely in |us, and that the relevant rendering to $tank takes place in the serf.

I tried virtualizing $tank printing in the king awhile back (in #1917), but the performance was atrocious -- presumably because we render $tank to $wall ((list tape)), and road promotion therefore involves lots of allocations. I think we should render $tank to $wain, and intend to experiment with that.

Finally, I enabled have-/need- printfs in the compiler recently, and, in my experience, the printing performance has been fine. But I'm sure we could do better, and this issue points to relevant questions.

@ohAitch
Copy link
Contributor Author

ohAitch commented Dec 20, 2020

Enabling either is simple, having confidence in the correctness and/or righteousness of either is somewhat less so.

Can you confirm whether or not doing so causes a subjective change in the experience of typing ut in dojo? If the runtime difference is like under 20% I would count the original issue as solved by |us rewrite, since I remember it being much more dramatic; though reducing use(/memory footprint+nonlocality) of tapes everywhere in the system is certainly a related laudable goal.

@zalberico
Copy link
Collaborator

There isn't anything actionable on this issue anymore. If we're concerned with printing performance we need to come up with some way to measure it and detect regressions. As is, there's not really anything that we can do to complete this issue. I'm going to close it. If there's some specific performance issue or action here then we can handle whatever that is separately.

@zalberico zalberico closed this as not planned Won't fix, can't repro, duplicate, stale Oct 25, 2022
@joemfb
Copy link
Member

joemfb commented Oct 27, 2022

@ohAitch's last question is perfectly actionable, and even includes acceptance criteria. Experimenting with home-road memoization would be good. There are lots of things that could be done to improve print performance, and it would be great to have measurements. But this issue is more specific than that, includes important historical context, and clear next steps.

@joemfb joemfb reopened this Oct 27, 2022
@zalberico
Copy link
Collaborator

Would the task be to:

  • Test the subjective experience of typing ut in the dojo
  • Enable memoization on the home road (not sure what that means, but presumably the two of you do)
  • Promote the memoization cache across roads
  • Test the subjective experience of typing ut in the dojo after these changes to see if it's subjectively less than 20% different (maybe measure this explicitly if we can) then this is good to close?

Also sounds like there may be another spinoff issue wrt rendering $tank to $wain but not sure if that's still relevant since it was about king?

The original context was from a change that can no longer be cleanly reverted which makes comparisons hard, but the issue to come out of this could be something specific about "Enable home road memoization to improve print performance" linking to this for context with specifics about correctness risks, etc. Maybe pulling the general value of "fast print performance" into the wiki and linking it back to this issue too. I could condense that down into one that links here. One of the goals of this is to pull out the important stuff so people don't need to read through old comments where only parts are still relevant, basically making it easier to read and understand quickly for people looking at an issue.

I'm not interested in just closing old issues (which is why I'm going through all of them), but a lot of time these old ones can be restructured in a way that's clearer. For example like this: #6033

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants