Skip to content

Conversation

primo-ppcg
Copy link
Contributor

@primo-ppcg primo-ppcg commented Aug 8, 2023

I understand that it's not desirable to move everything to corelib.c, especially things that can be implemented simply and concisely in Janet. However, for these three functions I think it may be warranted. Each of these functions compares multiple types, and are used in several places throughout boot.janet, and not only in compile-time expanded macros.

For example, in flatten:

(use spork/test)
(timeit-loop [:timeout 1] :flatten (flatten [[[1 2] 3] 4 [5 [6 7 8] 9]]))

master:

flatten 1.000s, 1.556µs/body

branch:

flatten 1.000s, 0.8098µs/body

Nearly half the time taken is spent checking the type.

For the interested, moving these functions increases the size of the executable by 128 bytes, at least on my system:
master:

$ du -b /usr/local/bin/janet
797104	/usr/local/bin/janet

branch:

$ du -b /usr/local/bin/janet
797232	/usr/local/bin/janet

@sogaiu
Copy link
Contributor

sogaiu commented Aug 8, 2023

master (0ea1da8):

$ janet ~/scratch/flatten.janet
flatten 1.000s, 2.227µs/body

branch (831f41a):

$ JANET_PATH=$HOME/.local/lib/janet ~/src/janet.primo-ppcg/build/janet ~/scratch/flatten.janet 
flatten 1.000s, 1.13µs/body

master (0ea1da8):

$ du -b `which janet`
824552	/home/user/.local/bin/janet

branch (831f41a): (needed to install to get the binary size reduced)

$ du -b `which janet`
820584	/home/user/.local/bin/janet

The binary from the master branch was bigger by 3968 bytes here.

When comparing the branch with ecc4d80 (1.30.0), the branch was larger by 128 bytes.

v1.30.0 (ecc4d80):

$ du -b `which janet`
820456	/home/user/.local/bin/janet

The changes didn't look like they'd have an affect on non-performance behavior, but I ran some tests on repositories anyway. As expected, I didn't notice any issues :)

Copy link
Member

@pepe pepe left a comment

Choose a reason for hiding this comment

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

LGTM

@primo-ppcg
Copy link
Contributor Author

We might also consider exposing lengthable?:

janet/src/include/janet.h

Lines 553 to 558 in 0ea1da8

#define JANET_TFLAG_BYTES (JANET_TFLAG_STRING | JANET_TFLAG_SYMBOL | JANET_TFLAG_BUFFER | JANET_TFLAG_KEYWORD)
#define JANET_TFLAG_INDEXED (JANET_TFLAG_ARRAY | JANET_TFLAG_TUPLE)
#define JANET_TFLAG_DICTIONARY (JANET_TFLAG_TABLE | JANET_TFLAG_STRUCT)
#define JANET_TFLAG_LENGTHABLE (JANET_TFLAG_BYTES | JANET_TFLAG_INDEXED | JANET_TFLAG_DICTIONARY)
#define JANET_TFLAG_CALLABLE (JANET_TFLAG_FUNCTION | JANET_TFLAG_CFUNCTION | \
JANET_TFLAG_LENGTHABLE | JANET_TFLAG_ABSTRACT)

@bakpakin bakpakin merged commit 7049f65 into janet-lang:master Aug 9, 2023
@primo-ppcg primo-ppcg deleted the bytes-indexed-dictionary branch August 10, 2023 05:50
This was referenced Aug 11, 2023
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.

4 participants