Closed patricksurry closed 2 months ago
I initially made changes to show 255+
in SEE
but then decided it was better to just implement the actual fix. See fixup_long_word, which either shuffles upwards one byte (if relocatable) or rewrites a new header (if NN). I added a couple of tests but it turns out that various words in the test suite already exercise this code (including }T
which is NN and >256 bytes). So I'm fairly confident things are good now. I made it conditional so that the minimal build (with TERSE) drops the extra code and just truncates length to 255 with NN flag.
I still need to review the comments per discussion above, but otherwise I think this is good to play with if you want.
ok, at least nominally "done" :)
Looks good so far. I'll see if I can find some time to really test this out this weekend.
I've played around with this, and it looks good. I'm going to merge the code now with the understanding that the documentation will need to be updated - I don't mind updating the documentation, but I won't have time to get to it immediately. The documentation in the code looks great, for anyone digging around under the hood.
cool - if you have any specific sections in the docs you know will need reviewed, feel free to make a ticket to capture that. otherwise i'll read through and update wherever it seems to make sense
On Thu, Sep 19, 2024 at 3:06 PM SamCoVT @.***> wrote:
I've played around with this, and it looks good. I'm going to merge the code now with the understanding that the documentation will need to be updated - I don't mind updating the documentation, but I won't have time to get to it immediately. The documentation in the code looks great, for anyone digging around under the hood.
— Reply to this email directly, view it on GitHub https://github.com/SamCoVT/TaliForth2/pull/131#issuecomment-2361963702, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABA5MKULGZC24XNVGE3ZPETZXMOFHAVCNFSM6AAAAABM3Z3XNGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRRHE3DGNZQGI . You are receiving this because you authored the thread.Message ID: @.***>
Sounds good. I'll leave a ticket open until I can get to it.
This implements #101 to reduce the storage needed for headers. I did the original work a couple of months ago so had a bunch of merge conflicts to resolve. So it's possible there are still some hangovers from that though tests are all now passing. The docs also deserve some updates, but I've tried to make the comments fairly comprehensive. I also took the opportunity to swap the length and status byte, so the status is now the first byte in the header. Since it's used so often it's nice to access directly from the data stack with
lda (0,x)
, but it also made me a lot more confident that I hadn't missed any subtle dependencies since any reference to either byte had to be changed.Since the built-in words collect all the headers together (disjoint from the word implementations) they typically have six byte headers instead of eight. Most user-defined words end up with four byte headers since the body immediately follows the header.
The total code size is now 20040 + 84 bytes as opposed to 20989 + 84 bytes in master (saving almost 1K or 5%). The typical savings on user code will be roughly double that. the compilation size of headers.asm shrinks from 6090 bytes to 5197 so almost all the savings is there: save 2 bytes/header for about 440 words which have avg 6 byte header and 6 byte name.
Execution speed is almost unaffected (benchmarks about 1/100th of a percent slower) since the changes mostly affect compilation. You can see the test suite slows by about 20% (149480031 to 179802023 ticks) since it's focused on compilation. By far the biggest cost is walking the NT list via find_nt_by_name which is slightly more complicated with the new headers - see comments in taliforth.asm. If we care about that, a tiny random replacement cache of recently used words would make a big impact.
Benchmark results: