Skip to content

Compress all translated strings with Huffman coding. #1121

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

Merged
merged 7 commits into from
Aug 17, 2018

Conversation

tannewt
Copy link
Member

@tannewt tannewt commented Aug 17, 2018

This saves code space in builds which use link-time optimization.
The optimization drops the untranslated strings and replaces them
with a compressed_string_t struct. It can then be decompressed to
a c string.

Builds without LTO work as well but include both untranslated
strings and compressed strings.

This work could be expanded to include QSTRs and loaded strings if
a compress method is added to C. Its tracked in #531.

This saves code space in builds which use link-time optimization.
The optimization drops the untranslated strings and replaces them
with a compressed_string_t struct. It can then be decompressed to
a c string.

Builds without LTO work as well but include both untranslated
strings and compressed strings.

This work could be expanded to include QSTRs and loaded strings if
a compress method is added to C. Its tracked in micropython#531.
@tannewt tannewt added this to the 4.0.0 - Bluetooth milestone Aug 17, 2018
@tannewt tannewt requested review from ladyada and dhalbert August 17, 2018 01:07
@@ -242,7 +242,7 @@ freedos:

# build an interpreter for coverage testing and do the testing
coverage:
$(MAKE) V=2 \
$(MAKE) \
Copy link
Member

Choose a reason for hiding this comment

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

check: this change is on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, it reduces the verbosity which is my preference.

Copy link
Member

Choose a reason for hiding this comment

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

got it!

@@ -102,20 +102,6 @@ const mp_obj_module_t mp_module_uerrno = {
};

const char* mp_errno_to_str(mp_obj_t errno_val) {
Copy link
Member

Choose a reason for hiding this comment

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

should this call mp_common_errno_to_str()? or remove gutted function so it doesnt get called by accident?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can't because we only want to allocate to the stack. decompressing here would require a heap allocation since the stack disappears on return. I think we do really want to call both.

Copy link
Member

@ladyada ladyada Aug 17, 2018

Choose a reason for hiding this comment

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

np - plz grep to check all calls to this are now calling mp_common_ :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

/V/E/circuitpython (huffman|✔) $ ag -Q mp_errno_to_str .
py/moduerrno.c
104:const char* mp_errno_to_str(mp_obj_t errno_val) {
127:const char* mp_errno_to_str(mp_obj_t errno_val) {

py/mperrno.h
143:const char* mp_errno_to_str(mp_obj_t errno_val);

py/objexcept.c
124:                    msg = mp_errno_to_str(o->args->items[0]);

const char* msg = mp_errno_to_str(o->args->items[0]);
const compressed_string_t* common = mp_common_errno_to_str(o->args->items[0]);
const char* msg;
char decompressed[50];
Copy link
Member

Choose a reason for hiding this comment

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

not 51 for final null?

Copy link
Member Author

Choose a reason for hiding this comment

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

50 is arbitrary since we don't know the length yet. That's why its checked below against the actual length (which includes NULL). It has to be declared in this scope so it survives down to the print call below.

Copy link
Member

Choose a reason for hiding this comment

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

@tannewt tannewt merged commit 5dd420f into adafruit:master Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants