espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.76k stars 741 forks source link

ESP32: Take advantage of JsVars being smaller. #2298

Closed GaryOtt closed 1 year ago

GaryOtt commented 1 year ago

ESP32 builds did not gain more JsVars when JsVars shrank in size because of the use of a literal in a calculation.

MaBecker commented 1 year ago

Yes - good point!

jumjum123 commented 1 year ago

Good point, very helpful for boards without additional PSRAM

Am Mo., 28. Nov. 2022 um 19:35 Uhr schrieb Gary Ott < @.***>:

ESP32 builds did not gain more JsVars when JsVars shrank in size because of the use of a literal in a calculation.

You can view, comment on, or merge this pull request online at:

https://github.com/espruino/Espruino/pull/2298 Commit Summary

File Changes

(1 file https://github.com/espruino/Espruino/pull/2298/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/espruino/Espruino/pull/2298, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPYNHWJFN5ZHGBMIQUAJBLWKT3ONANCNFSM6AAAAAASNUMBRA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

gfwilliams commented 1 year ago

Nice - that's a good spot! What is the size of a JsVar?

Please could you also tweak the line a bit below:

if(heapVars > 20000) heapVars = 20000;

to also add:

int maxVars = (1<<JSVARREF_BITS)-1;
if (heapVars>maxVars) maxVars = heapVars;

I'm not 100% sure what ESP32 does, but you can see the size of the JsVar determines how many vars you can have (based on the size of addresses) at:

https://github.com/espruino/Espruino/blob/master/src/jsutils.h#L220-L229

So realistically if the var size isn't 16 bits then if you ever had more than 8191 vars then things would crash very badly.

rblakesley commented 1 year ago

I think you've got a typo there – did you mean this?  ;-)

int maxVars = (1<<JSVARREF_BITS)-1;
if (heapVars>maxVars) heapVars = maxVars;
gfwilliams commented 1 year ago

Yes - thanks :)

GaryOtt commented 1 year ago

sizeof(JsVar) returns 13.

Good spot, I'm on it.

gfwilliams commented 1 year ago

:o Thanks for checking up! I just checked on a standard ESP32 and we only use 2200 vars with BLE enabled, but on PSRAM enabled ESP32s things would get super unstable

GaryOtt commented 1 year ago

Well, that's interesting! That limits to 4095 JsVars. Users who don't disable the bluetooth module will have little left over.

I've been using custom builds of Espruino with unused modules removed that give me 5300 JsVars for quite some time. I have not seen much instability but it's very difficult to know what my applications peak usage is. I often use process.memory() to determine what the base memory consumption (about 3000 JsVars). I think peaks are around 1500 JsVars higher because there are certain operations that I log process.memory().gc to know how much is cleared after they complete. I would have thought I should have seen more crashes.

Would it be safe to extend JSVARREF_BITS by one more bit?

gfwilliams commented 1 year ago

That limits to 4095 JsVars.

That's interesting - based on the code above it seems 8191 should be possible - I guess JSVAR_CACHE_SIZE just needs setting to the max you think can be allocated.

Would it be safe to extend JSVARREF_BITS by one more bit?

Not directly - the bits are packed together in a very specific way - but based on https://github.com/espruino/Espruino/blob/master/src/jsutils.h#L220-L229 it should still be able to do 8191 with 13 bytes (although possibly the lack of references may cause issues).

It'd also be possible to add another option for 14 bytes, 16383 vars?

#elif JSVAR_CACHE_SIZE <= 16383 // 14 bytes
    #define JSVARREF_BITS 14
    #define JSVARREFCOUNT_BITS 8 // 64 - 13*4
    typedef uint16_t JsVarRef;
    typedef int16_t JsVarRefSigned;
GaryOtt commented 1 year ago

I like the 14 byte JsVars option. I think it is a solution that works for all because it still allows a 12.5% increase in JsVars while removing potential instability.

Please excuse me while I take my time to properly understand code which is new to me. The more I learn the more questions I have. I think an opportunity to use JSVAR_CACHE_SIZE somewhere else may have been missed. I'm checking that out before I update the PR.

GaryOtt commented 1 year ago

@gfwilliams Sorry, I don't know if you want to hold off pulling this. After pushing I have seen something unexpected.

This shows the limit is working as expected but I don't understand why blocksize is showing 13 rather than 14:

image

EDIT: I see what I have done wrong. I'm an idiot.

GaryOtt commented 1 year ago

It is all working as it should.

Below shows off builds with the Graphics, Neopixel and Bluetooth modules disabled on an ESP32 without PSRAM. The only difference is the 'variables' parameter in boards/ESP32.py:

variables: 4095 I made this the default setting so stock builds will still use 13 byte JsVars with up to 255 references. Those users should get a few more JsVars.

image


variables: 8191 Peek number of variables in this particular scenario but limited to 7 (EDIT: 15) references per variable. image


variables: 16383 In this case, number of JsVars has gone down because they're 14 bytes each but each can have 255 references making it a much safer choice for use cases that have the available memory.

image

gfwilliams commented 1 year ago

This looks great - thanks!