Don't delete variables

[atjn]

Related: #345

I suggest disallowing variable deletions in the official apps, since they are not allowed per the Ecmascript standard. I think that allowing developers to use a core language feature differently from the standard is a recipe for disaster, as we can suddenly have programs that only work in a specific runtime, even though they are written in JS.

Another reason to remove this code, is that it shouldn't really do anything. Keeping unused data in memory does not have any cost, as long as there is still some free memory. If there is no free memory, the garbage collector should be able to efficiently detect and remove unused data.
Most JS implementations that allow deletion of variables do not actually delete them from memory, as it would unnecessarily interrupt the GC. The GC might know that a different variable will free up more memory, or would free up memory at a better address, or it might know that there is no need to free memory at that time. Then why would you force it to spend cycles freeing your variable?

Of course you know better than me how the Espruino GC works. If you still think that variable deletion is a good feature, I would suggest adding compiler hints. They could look something like:

// espruino free var "foo"

Because they are comments, they can be as non-standard as you wish, while keeping comatibility with javascript.

[bobrippling]

I would be interested in Gordon's opinion on this - obviously it requires changes to the interpreter and there's a bunch of other non-standard behaviour that should be considered if we were going down this route

Additionally, this route would be a simpler change, without needing comment parsing, etc:

 myGlobal = "taking up memory";
-delete myGlobal
+delete global.myGlobal

[atjn]

@bobrippling I like that example, it is only using existing and valid properties of JS but also providing a hint to the GC that the variable is now unused. Usually it would be a big no-no to just set globals everywhere, but given how small Espruino programs are, it is probably not a big deal. This is of course only relevant if the GC actually needs these hints, otherwise I refer to the argument that it is better to just let the GC garbage collect when it wants to.

[atjn]

Another solution could be to add a global function, something like E.free() to the library:

const memoryHog = "lots of mem....";
E.free(memoryHog);

Other JS engines won't have this feature, but it is very easy to add a global E object with an empty free object, and then the program will work fine. This is in contrast to the delete method, which is a syntax error so it is impossible to run that code without making manual modifications to the source first.

[atjn]

Another thing: If you decide to have a non-standard "delete" method, I would strongly recommend that you don't allow the deleted variable name to be used again. For example, if you allow code like this:

const memoryHog = "lots of mem....";
E.free(memoryHog);
console.log(memoryHog) // logs 'undefined'
const memoryHog = "new value"; // no problem

..then the code is still invalid for other runtimes that don't have special handling for the free method. The code should work like this:

const memoryHog = "lots of mem....";
E.free(memoryHog);
console.log(memoryHog) // runtime error: variable does not exist
const memoryHog = "new value"; // syntax error: variable cannot be redeclared

The only situation I can see that would be acceptable, is redeclaring a variable with var, so:

var memoryHog = "lots of mem....";
E.free(memoryHog);
console.log(memoryHog) // STILL A RUNTIME ERROR: variable does not exist
var memoryHog = "new value"; // no problem

[atjn]

@bobrippling To extend on your solution, it is also possible to do this:

 let myLocal = "taking up memory";
-delete myLocal
+myLocal = undefined; // implicitly free memory

This also works in plain JS, so we don't have to introduce any complex non-standard logic.

I would be happy to create a new PR that replaces the delete operations with these patterns. But once again, I would only do that if you can confirm that this is indeed necessary for the GC. The rule of thumb is that this kind of manual memory management is completely unnecessary, but I can understand if the unique nature of the Espruino GC (needs to be tiny, have small memory footprint) means that it actually could benefit from these patterns.

[gfwilliams]

Hi, sorry for the delay responding, I was away last week.

Keeping unused data in memory does not have any cost, as long as there is still some free memory. If there is no free memory, the garbage collector should be able to efficiently detect and remove unused data.

This is not how a garbage collector works unfortunately - if the variable is referenced in any way it will never be freed.

-delete myLocal
+myLocal = undefined; 

This doesn't entirely work either - while the contents of the variable is freed, the variable name is not, so still takes up some space.

While you may have some free RAM on Bangle.js 2, much of this code has to run on Bangle.js 1 as well, where RAM is at a real premium, and especially for the 'default' apps there's often been very considered, careful use of delete to ensure that we free the most possible RAM (and also ensure that we don't leak memory when 'fast load'ing).

This PR as-is, just cannot be merged because it'll almost certainly cause memory leaks all over the place.

I'm all for enabling sensible linting (like your changes for octal), but in this particular case I don't think this makes sense. I don't believe I've seen a single error so far in this repo caused by bad use of delete. I know it's not what the ES standard says, but to try and run JS on a device with 64k of RAM (for the Bangle.js 1 case) there have to be a few concessions, and I think this is one of them.

[atjn]

Keeping unused data in memory does not have any cost, as long as there is still some free memory. If there is no free memory, the garbage collector should be able to efficiently detect and remove unused data.

This is not how a garbage collector works unfortunately - if the variable is referenced in any way it will never be freed.

If you run the following program 200 times in a for-loop in the V8 engine:

async function f(){
    let v = "";
    for(let e = 0; e < 1000000; e++) v += Math.random() > 0.5 ? "k" : "o";
    
    await new Promise(resolve => {setTimeout(resolve, 1000*40)});
    const k = "";
}

Then you will see that the GC periodically frees the variable v from memory, even though v is still referenced in the active functions. Here is the memory usage while running the for-loop:

If you use v at the end of the function, you can see that the GC is unable to free the memory:

- const k = "";
+ const k = v.at(-1);

Memory allocation:

So I think my argument is valid: In a normal JS engine, the delete operator is not necessary, because the GC can remove those variables automatically.

I do understand if the Espruino GC is less advanced, and you think that this pattern is the best workaround for that limitation :)

[atjn]

@gfwilliams would it be okay if I create an issue to discuss a different mechanism for deleting variables? I would be happy to do most of the work to spec and implement the feature, but it obviously requires that you spend some time looking at it. I think it is worth the time spent, but I understand if you disagree :)

[gfwilliams]

In the example you're giving, it's a local variable in an async function - when the function scope is no longer referenced then it can be freed. While you have the setTimeout, I guess Node is smart enough to see that the remainder of the function that could reference it actually doesn't, and so it can be freed.

On Espruino, when a function is defined, it references the scope it was defined in which will include any variables in that scope, regardless of whether the function uses them. Because the JS isn't compiled, Espruino has to assume that the function could execute any code, so could reference anything in the scopes that it was defined in (it's like if your function contained eval, Node.js would be forced to keep all vars in the scopes around). As a result the GC will only happen in Espruino when the original function and all functions that were defined inside are no longer referenced.

I get that you don't like a language feature being used differently to the spec, but I don't see this will really cause bugs (and looking at the ES3 spec it looks a lot like deletion of vars was allowed at that point, but it could be it's just not clear). I mean, if we have a E.delete function all you're doing really is ensuring that the JS code will be unable to run at all on different engines, when right now code will run (outside of strict mode) but maybe just will use a little bit of extra memory.

We also need to be backwards compatible so that (assuming something was added in 2v22) Bangle.js users with firmware 2v21 and earlier can at least run the apps in the app loader - the potential for breakage for users while managing that changeover far outweighs any benefit of changing this.

So I think with this particular one, I'd rather not consider changing it at the moment.

[atjn]

@gfwilliams feel free to ignore this if you are done with the subject, but I do not agree with your conclusions.

I mean, if we have a E.delete function all you're doing really is ensuring that the JS code will be unable to run at all on different engines, when right now code will run (outside of strict mode) but maybe just will use a little bit of extra memory.

I think you got this backwards. If you use delete, you are not able to run the code at all in any modern JS framework, because it only supports strict-mode code. The general trend is towards more JS code being written in strict-only contexts, which is a good thing. On the contrary, if you made an E.delete function, it would be easy to add a global E = { delete: () => {} } to the variable scope before running bangle code, and that would make the code run perfectly in any environment.

We also need to be backwards compatible so that (assuming something was added in 2v22) Bangle.js users with firmware 2v21 and earlier can at least run the apps in the app loader - the potential for breakage for users while managing that changeover far outweighs any benefit of changing this.

I understand and agree, but I see no reason why you can't add it now so it will be able to benefit you in the future. Then when some amount of time has passed, you can write the call such that it fails silently if not defined (E.delete?.()).

[gfwilliams]

I see no reason why you can't add it now so it will be able to benefit you in the future

E.delete(var) wouldn't work because function parameters in JS pass the value, not the name. As far as E.delete is concerned, a={};b=a; E.delete(a) is the same as E.delete({}) so it wouldn't know whether to delete a or b. So we'd have to do some nasty hack - maybe E.delete("variablename") which the linter or other JS tools then wouldn't understand at all.

And then we're also constantly pushing against the available flash memory in devices. Sure, adding that function will 'only' add maybe 150 bytes, but then on those restricted builds (Micro:bit 1, Bangle.js 1, Original Espruino) where I'm ending up spending a lot of time making changes like this just to save 30 bytes to keep everything compiling, it's just one more step towards the point where I have to pull another feature out of a build to make space - all for something which doesn't add any new functionality.

I know it's not ideal, but there are loads of conflicting requirements when trying to write something for very small, embedded devices, and not everything can be perfect.

[atjn]

I see. Thank you for taking the time to discuss these things, I appreciate it!