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

jets: tune slaw, scot jets #425

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

mikolajpp
Copy link

Introduction

This PR accomplishes the tune slaw/scot jets grant.
Slaw and scot jets were improved and extended, and their performance
benchmarked.

+scot serializes atoms to strings, +slaw parses strings to atoms, returning a unit of
the parsed atom.

> (scot %p 123)
~.rux

> (slaw %ux '0xcafe')
[~ 51.966]

> (slaw %ux '0xcaf.e')
~

The Hoon code of +scot and +slaw was scrutinized for
each jetted path. The issues found - bugs in the Hoon parser, as well as a jet mismatch, are addressed in a later section.

The work is based on Joe's PR #291, which provided scot jets for %ud, %ux, %uv and %uw.

Scope of the work

In the scot atom printing jet, %ui, %da and %p were added.
(There are existing %da and %p +scow jets, these however were disabled in December 2020 due to obscure memory errors.)
In the slaw parsing jet, auras %da, %p were reimplemented, and %ui, %uv, %uw, %ux were
added.

Unit tests were implemented for all jetted auras, both in Vere, as well as in Arvo tests in the Urbit repo

The scot, slaw aura occurence tables accross desks available in a
standard installation (base, garden, group, landscape and talk) are as follows.
The jet contributions of this PR are marked with a star.

aura scot jet
⭐ %p 444
⭐ %da 281
%ud 144
%uv 123
%ux 56
%uw 31
⭐ %ui 30
%tas 7
%q 5
%t 3
%if 3
%dr 3
%x 1
%rs 1
aura slaw jet
⭐ %p 53
⭐%da 22
%ud 16
⭐%ux 15
⭐%uw 7
⭐%uv 6
%t 4
⭐%ui 3
%tas 2

Memory Checks

Past attempts to implement and improve upon slaw/scot jets ended up
hitting memory problems of unknown source, see urbit/urbit#5161. Here, the implemented code was scrutinized carefully, noting the allocation and freeing of each of the introduced variables. Unit testing in jets_tests.c with u3m_grab did not report any memory leaks.

Benchmarks

The slaw, scot benchmarks can be found in a Julia notebook: https://mikolajpp.github.io/aura-benchmarks.html

Issues

Incorrect Hoon parser behaviour

During the examination of the relevant Hoon parsing arms several problems were discovered. These are reported in a PR in the Urbit repo: urbit/urbit#6628.

%p parsing jet mismatch

There is a jet mismatch in the existing slaw %p parser.
Below examples should not parse, but with the +slaw jet enabled
they parse on the current runtime.

> (slaw %p '~dozzod')
[~ 0]
> (slaw %p '~doznec')
[~ 1]
> (slaw %p '~doznec-litsem')
[~ 1.627.693.323]
> (slaw %p '~dozzod-doznec-litsem')
[~ 1.627.693.323]

This is fixed with a rewrite of the
slaw %p jet.

Acknowledgements

I thank @joemfb for making his branch available as a starting point for
this work. I thank @sigilante for his patient mentoring on this project.

@mikolajpp mikolajpp requested a review from a team as a code owner June 1, 2023 02:16
@mikolajpp mikolajpp changed the title Tune slaw, scot jets jets: tune slaw, scot jets Jun 8, 2023
@jalehman jalehman requested review from joemfb and barter-simsum June 26, 2023 15:53
@matthew-levan
Copy link
Contributor

@sigilante will you comment here with your thoughts please? This PR has been open for a month or two now but seems to be stuck.

@sigilante
Copy link
Contributor

The code here is fine as far as it fulfills the bounty requirements.

The blocker is the jet mismatch because of incorrect atom parsing behavior in Hoon—these jets are more correct and/or require clarification on what we will permit atom behavior to be. We need a decision from core dev on what to do about that, which is I believe addressed in a different issue or gist.

@mikolajpp
Copy link
Author

The new jets behavior is matched by urbit/urbit#6628. The merge must be coordinated with the other PR.

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

Successfully merging this pull request may close these issues.

3 participants