Big update to prettier-extension-monkeyc

But the point is not to add import Toybox.Application, but to add Toybox.Application.Properties AND remove Application from the place where Application.Properties.foo is called.

Ah, so it's even more complicated:

In File1 I have import Toybox.Application.Storage, but it's not "carried over" to File2 in the optimized code. When I add the import to File2, then the warning disappears, so I think this is another bug (besides the wrong file/line_number in the warning), that the import is lost. This is especially strange because it doesn't seem to add to the code, and because I see sometimes in the generated code that all the functions were eliminated from a file but the imports are being kept

flocsy🤠 said:

When you compile this with excludeAnnotation foo then there's a warning:

If you compile without the optimizer, it fails to build unless you turn off the type checker, and add --Eno-invalid-symbol to the command line. So its a reasonable warning to issue...

But I've always been in two minds about this. Most compilers (including Garmin's) issue diagnostics based on the unoptimized code, so even if something gets optimized away, you still get the diagnostics that would have happened if it had remained.

But then there are cases like:

if ($ has :notHere) {
  notHere();
}

It seems pretty stupid to issue the error in that case. So I go to some lengths to avoid issuing diagnostics for code thats going to be removed. But in your case, by the time cond() has been inlined, the diagnostic has already been issued.

I could turn off (most) diagnostics during optimization, and then do one last pass that only issues diagnostics (I already have that pass - its what produces the "live" warnings in the editor). But I tried that and it sometimes misses real issues. eg consider an inline function thats declared as returning Number, but actually returns Float. Once it's been inlined, there's usually nothing left to typecheck. eg

function num() as Number {
  return 42.0;
}

function foo() as Void {
  var x = num().toString();
  System.println(x);
}

becomes

function foo() as Void {
  System.println((42.0).toString());
}

flocsy🤠 said:

But the point is not to add import Toybox.Application, but to add Toybox.Application.Properties

Yes, I get that. What I'm saying is that there aren't many opportunities to do that.

Assuming people use the standard imports (ie 'import Toybox.<blah>'), then there are literally only three symbols like this. Application.Properties, Application.Storage and and Time.Gregorian.

What Im saying, is that the only way this could be a big win is if someone didn't use import at all. In that case they would have to write everything as "Toybox.Foo", and I could add the "import Toybox.Foo" and replace Toybox.Foo with Foo, as you're suggesting.

But since nobody does that, there's very little opportunity for an optimization here. There's also the precedence issue.

Once you add an import or using, the imported symbol takes precedence over all other module scope names.

import Toybox.Application;

class Storage {
 //...
}

function foo() {
    var x = Application.Storage.getValue("foo");
    var y = new Storage();
    ...
}

If I add "import Toybox.Application.Storage" to the above, "new Storage" now refers to Application.Storage, not $.Storage.

Not impossible to check for - but it does require a lot of analysis of the code - for a tiny win. But yes, I have thought about doing it. In fact we discussed it a long time ago when I first introduced inlining; partly because the original inliner could introduce bugs by inlining from a file with an import to one without it. Which seems to have struck again, in a less serious way:

flocsy🤠 said:

so I think this is another bug (besides the wrong file/line_number in the warning), that the import is lost

So what's happening here is that when the inliner inlines something, it checks to see if all the symbols resolve in the new context, and that they resolve to the same things as in the original context. And if they don't it qualifies the names until they do (and yes, I *could* add imports, and probably should eventually, but *that* involves doing all the checking described above).

In this case, because you're compiling with compiler2, it sees that the symbol *does* resolve to the same thing, so it doesn't need to fix it. But then it issues a warning because if you compiled with compiler1 it wouldn't work...

I can't reproduce this now. I think there might have been another error that caused the garmin compiler not even to get to the point where it would warn about notHere not being there.

1. I think exactly because most people who use the optimizer will have something like:

(:no_api2_4, :foreground, :no_background, :inline)
function setConfig(key as PropertyKeyType, val as PropertyValueType) as Void {
    // getApp().setProperty(key, val);
    // Application.getApp().setProperty(key, val);
    // App.AppBase.setProperty(key, val);
    MyApp.setProperty(key, val); // -4
}
(:api2_4, :foreground, :no_background, :inline)
function setConfig(key as PropertyKeyType, val as PropertyValueType) as Void {
    Application.Properties.setValue(key, val);
    // Properties.setValue(key, val); // -6
}

and then they use setConfig in many places in the code (and possibly getConfig even more times), this could gain 6 bytes * times it's inlined.

2. But if you can't now / don't want to do that optimization, then can you add a warning for these known cases?

I'm actually seeing something slightly different from what I expected; and different from what you reported too.

At the start of this, you said that you didn't like adding "Application" to Properties.getValue in response to the warning because it added 6 bytes to the code. Which makes sense - thats exactly what I thought would happen. Each component in a dotted path seems to add 6 bytes.

But when I tried it (without adding "import Toybox.Application.Properties"), they both come out the same. And inspecting the byte code, I see they both produce exactly the same code. In other words, with compiler2, if you "import Toybox.Application", you can *say* Properties.getValue, but the compiler treats it exactly the same as if you'd said Application.Properties.getValue.

Once I add "import Toybox.Application.Properties", then if I say Application.Properties.getValue(), it still produces the same code, but Properties.getValue() is indeed 6 bytes smaller.

But now I'm wondering what else must have changed... after the import, there's a "shortcut" to Toybox.Application.Properties that wasn't there before. And it's a shortcut that works at runtime. So what else must have changed? There must be a table somewhere telling it how to lookup Properties. There are lots of other sections in the binary - maybe one of those got bigger; and maybe that also contributes to the application's memory footprint. Anyway, Im not really sure how to get an answer to that, but I'm wondering if its *really* a win...

To me it sounds exactly what I meant to report. Even if there are some tiny differences in how we teel things (because you understand much better how this works) the main point was that adding an import and removing "Application." seems to decrease the code size by 6 bytes.

I don't really understand your last paragraph. I understand (and we even discussed it about a week ago, that sometimes a change might decrease code size but increase memory usage at runtime) that it's not enough to look at code+data size, but do you really think that something else could grow in size?

I compared the generated mir files:

diff Utils.without.import.Toybox.Application.Properties.mir Utils.with.import.Toybox.Application.Properties.mir 
4a5,6
> [ @file = "bin/optimized/group002-debug/source/source/Utils.mc"; @line = 3; ]
> import Toybox.Application.Properties;
21,25c23,27
<     %tmp.2 = getv ? :Properties;
<     %tmp.3 = getv function %tmp.2 :setValue;
<     %tmp.4 = "-";
<     %tmp.5 = "";
<     invoke %tmp.2 %tmp.3(%tmp.4, %tmp.5);
---
>     %tmp.1 = getm $.Toybox.Application.Properties;
>     %tmp.2 = getv function %tmp.1 :setValue;
>     %tmp.3 = "-";
>     %tmp.4 = "";
>     invoke %tmp.1 %tmp.2(%tmp.3, %tmp.4);
27,28c29,30
<     %tmp.6 = false;
<     ret %tmp.6;
---
>     %tmp.5 = false;
>     ret %tmp.5;
34,35c36,37
<     %tmp.7 = exception;
<     push %tmp.7;
---
>     %tmp.6 = exception;
>     push %tmp.6;
38,39c40,41
<     %tmp.8 = dup %tmp.7;
<     lputv %ex.1 %tmp.8;
---
>     %tmp.7 = dup %tmp.6;
>     lputv %ex.1 %tmp.7;
42,43c44,45
<     %tmp.9 = true;
<     ret %tmp.9;
---
>     %tmp.8 = true;
>     ret %tmp.8;
50c52
<     throw %tmp.7;
---
>     throw %tmp.6;

But I leave it to you to understand what it means.

But what's more interesting is that it the build-info.json seems to be 400 bytes different in size, but it looks like it's only because of the warning you report.

Both the prg and debug.xml differ in content but are the same in size.

flocsy🤠 said:

do you really think that something else could grow in size?

Yes. It definitely could, and I'm really surprised that it doesn't appear to do so. Compiler1 didn't really do much code analysis; it pretty much just output whatever you wrote verbatim. But Compiler2 is much smarter. It definitely knows what Application.Properties refers to; so if there's a cheaper way of referring to it without *any* other changes, why doesn't it use it?

The code without import does

  spush Toybox_Application
  getm
  spush Properties
  getv

where spush pushes a symbol, getm appears to get the module corresponding to the given symbol, and getv looks up the given symbol starting from the object on the top of the stack.

The code with import just does:

  spush Toybox_Application_Properties
  getm

So something has to tell the runtime that Toybox_Application_Properties points directly to the Properties module; there must be an entry in a hash table somewhere.

Anyway, I've been digging around to try to see what else might have changed, and Im not finding anything - so maybe the hash table entry is always there, whether you import or not, and the compiler is just not smart enough to use it...