Merge arguments and local scope

With Lucee 6.1 we focus on performance that includes the following point

  • improve/shrink the bytecode generated by specialisation
  • improve repeatable task like checking if a template exists
  • improve start up speed
  • adding tags/functions that allow better more performant enviroment interaction like directoryEvery or FileEvery

The goal is that most of this points are backward compatible, so anyone benefit from them by simply updating to that version without any change to the config or code necessary.

One idea is to merge the local and arguments scope, because they are mostly redundant. Problem is that would not be backward compatible and need a settings to enable it. But that means most user will most likely never enable this and because of that not benefit from it.

So any idea how we could improve this without breaking backward compatibility or any other improvment you can think of is warmly welcome.

1 Like

Could you make it so that the arguments scope and the local scope are just the same reference? So, keep both of them, but one is really just an alias? Not sure if that would remove the performance boost you are trying to achieve.

You’d lose the ability to have local.value and arguments.value be two different things. But, having those be two different things feels like a problem to begin with.

Actually, now that I’ve stopped to think about it, one of the big problems we might run into is that the arguments scope is kind of both an array and a struct. Whereas local is just a struct (I believe). This allows the arguments scope to be used with both named and ordered inputs. And, checking the .len() of the arguments means something.

You are talking about 6.1, but has 6.0 officially been released as stable yet? The last announcement I saw was a 6.0 RC, but nothing since then?

Yeah Ben already points out one thing that might get in the way here: they’re two different types (and for two different purposes), so your starting position of “merge the local and arguments scope, because they are mostly redundant” might not be quite as clear cut as you present it as.

Secondly, CF already kinda deals with this (ish), and Lucee is not at feature-parity with this:

Arguments are supposed to already be in the local scope. Not the way you meant, but still… maybe address that cross-compat shortfall first.

Lastly, “merge the local and arguments scope, because they are mostly redundant” is rather subjective, and doesn’t really define a problem that needs solving. What’s the actual - objective - well-defined problem yer trying to solve here? What happens if you don’t do this work? Anything? Having a well-defined problem that needs solving should be the starting point for any endeavour like this, I think.

As I’m thinking about the merits of this work, I did a quick shufti through Jira:

type=bug and status not in (Deployed, qa, Rejected, Resolved) and priority > major

(https://luceeserver.atlassian.net/issues/?jql=type%3Dbug%20and%20status%20not%20in%20(Deployed%2C%20qa%2C%20Rejected%2C%20Resolved)%20and%20priority%20>%20major)

There’s 103 bugs there marked higher than “Major”, representing real-world issues ppl have taken the time to raise, because they’re being impacted by them.

Wouldn’t sorting out some of those be a better use of dev time than stuff like this?

4 Likes

@bennadel Lucee comes with a setting to merge the form and url scope, if enabled one is just an alias to the other, as you suggested also for this case. problem with that solution is backward compatibility, the situation that someone has the same key in local and arguments scope with different values would no longer work. if merged we would let the local scope point to the arguments scope.

@Leftbower Lucee 6 is released

@AdamCameron if it would be clear i would never started that thread ;-). ATM i see no valid solution for this, this is actually something bugging me for a long time. Having a setting in the Admin, that is off by default is not worth the effort in my opinion, because most people will never enable it.
I’m aware that the implementation in Lucee and ACF differs, in Lucee this scopes are completely separated and in ACF the arguments scope is inside the local scope with special behaviour.
The following code shows the only 2 cases that differs from ACF to Lucee because of this TryCF.com

We will for sure not change that behaviour in Lucee to be ACF compatible, because nobody ever did complain about this difference so far, the only complain we got about the local scope in the past, was that Lucee (or better Railo back then), did allow to set "var " not only at the top of the function, but later ACF did change that behaviour, so today that works the same way as in Lucee.

The reason for a possible change is simply performance, if you address an argument in the scope without defining the scope, it always checks first the local scope before the arguments scope and because in a regular application this happens a lot, it is worth thinking about improving this behaviour. the whole function handling is under consideration.

For example one idea is that Lucee detects if the arguments scope get overwritten in a function and then handles it differently, because this is a rare occasion it is worth to handle a “readonly” arguments scope in a different way. Also a function is called more than once normally, so some initial overhead makes sense.

Thanks for the clarification: makes sense.

Are we getting into micro-optimisation here? What sort of gains are we looking at?

My position on these things that potentially might have server-level or app-level switches is “no, never do that”. Either make the change, or don’t make the change. So the “make the change” would now need to be for Lucee 7, as it’d be a breaking change.

Why not have the switches? Cos it changes code execution behaviour, and third-party library authors need to either mess around supporting both behaviours, or - more reasonable - actively don’t support the non-default behaviour at all. So given that… it makes the optionality pretty unusable, so a waste of time.

In 2023 anyone that might potentially be going “ooh I wish we could eke out an extra few microseconds by having that scope-lookup in situation improved” is not gonna be developing their apps in a vacuum… they will be depending on third party apps. And even if they did… just scope the variable! Job done. Lucee doesn’t need to wipe any arses here, possibly.

The only ppl these optional settings work for are those invisible sorts who still develop apps like it’s 2003, and write the entire thing themselves. With the six tags they are comfortable with. And they’re not likely to be savvy enough to even understand what the optimisation is for, let alone be going “I wish that optimisation existed”.

I still think it’s solving a perceived problem, not a real one.

1 Like

If I can comment on the idea of “switches” for moment. The biggest hurdle to many of these larger changes is that it becomes problematic the moment you pull in a 3rd-party library / framework. It’s one thing if your app is going to use the new standard; but, what happens if you want to use ColdBox or CFWheels or FW/1 or what-have-you? Then, their code might be making assumptions that your code is not.

This is the reason I’ve never tried using the switch for Java’s RegEx engine (instead of POSIX). Or, tried enabling null support.

All to say, it would AWESOME if we could start enabling switches at the component level:

component
    argumentsScope="merged"
    localmode="modern"
    regexEngine="java"
    nullSupport="enabled"
    {
    // ....
}

Then, I could be fully sure that the stuff I’m using inside my code is correct, and I don’t have to worry about 3rd-party libraries.

Kind of a tangent, but just thinking out loud.

4 Likes

i totally agree and i have no breaking change in mind that changes the default behaviour, a breaking change always will be behind a switch and because most people will not use it that is more or less not an option for me. I’m coming away from all this switches in Lucee.

yes the optimisation for a single function call will be in the micro seconds, but a regular application makes a shitload of function call, so a regular request can improve in milliseconds.

My first step was to find out where Lucee spends most of its time in the code, and calling functions is a big part of it. Then i tested the impact by double certain steps of a function call, that way you can measure what the impact is by seeing how much slower a certain request gets. This told me that there is potencial.

The Lucee compiler already detect if the argument|local scope is passed outside a function and if not for certain this scopes get recycled what also reduces the time wasted for the garbage collector and memory in general. So improvement not necessary is faster code, it can also be, spend less memory, what in the end is the same.

But we for example improved already the byte code generated for set and get of most variables. it is not only smaller bytecode, we could also remove some steps by making the bytecode generated less generic and more specific for at least 95% of all variables.

A change is only of interest if it at least brings a benefit in milliseconds for a average request.

2 Likes

@bennadel have you ever gone “gee I wish the arguments and local scopes were merged? That’s a real pain point for me”.

I have never worked on a huge project (traffic-wise), but have def worked on medium-sized projects that bang through hundreds of requests per second, and I’ve never once thought that. It’s never been the source of either a code-clarity or performance concern for me.

As for the regex engine thing (gratuitously OT for this thread)… that’s unlikely to be how I would solve that issue. And, TBH, I don’t think it’s an issue to be solved anyhow. Java’s regex implementation has been there since 1.0, and it’s unlikely to have breaking changes appear now. Also obvs it’s what yer tests are for (chortle), when evaluating upgrade risk…

I’d prefer that they not be merged. I often use the local scope instead of making changes to the original argument. This enables me to easily debug by returning both the arguments & local scopes in the response to determine what is happening.

I’ve also been explicitly scoping the variables to leave nothing to chance. Incorrectly or commonly named unscoped variables have been the source of some problems we’ve encountered in the past.

2 Likes

Interesting. Can you give an example please?

I would strongly disagree with this approach, as the change being proposed is not only breaking for existing apps but breaks encapsulation in a big way - and allows for unexpected mutation of the passed in args- especially if the argument collection being passed in is positional or dynamic.

Take for example:

myComponent.populate( argumentCollection=form ).save();
function populate(){
// now added to the form scope with this change
var foo = "bar";
arguments.keyArray().each( ( key ) =>{
///... do stuff with keys... but if I had a form key of `foo`, I just overwrote it above
});
return this; 
}
}
6 Likes

Not to hijack the thread… but seems there would’ve been some kind of announcement with a major release? I cannot find any? Apologies if I missed it.

Anyways, glad it’s out!

2 Likes

I tend to agree with @jclausen merging the scopes has the potential to break things in many ways. Here’s a few examples I thought of:

function doSomething(value) {
   var value = some_default;
   if ( isValidValue(arguments.value) ) {
       value = arguments.value;
   }
   //...
}

If the scopes were merged, local.value and arguments.value would always be set to some_default.

It certainly would be better to use a different local variable name in the above example, but my point is code like the above is possible, and I’m sure out there.

There is also a lot of old code out there written before we had local scope (CF8 I think), that looks like this:

var local = {};
local.something = 1;
local.another = 2;

What would happen to the arguments scope there? I would imagine it would be overwritten.

I have a feeling Micha already thought of all these things:

Is there a way where you can avoid creating either the arguments or local scope objects in the bytecode if you detect that they are not used within the function. That might allow you to optimize it in some cases, and also not break anything.

For my own clarification here…
When “local” is mentioned here…

Is it local “scope” or a struct named “local” in some scope?

we did slow rollout with a silent release, but official announcement will follow soon. In development we always are couple steps ahead.

I’m aware of the special nature of the argument scope (i have implemented it :wink: ), of course what ever we do that will not be lost.

1 Like

@pfreitag yes if they are simply merged then there must be a switch to enable it, because it breaks backward compatibility.
A solution could be, you only have one scope, but every entry (key value pair) in the scope can have 2 values, one for “local” and one for “arguments”.

So when you do for example #susi# #local.susi# #arguments.susi# inside the function lucee internally does the following (simplified)

// getting "susi"
UndefinedScope {
   function get(name) {
      localArgValues=localarg.get(name);
      if(localArgValues!=null) return localArgValues.first(); 
      throw "key ["+name+"] not found";
   }
}
// getting "local.susi"
LocalScope {
   function get(name) {
      localArgValues=localarg.get(name);
      if(localArgValues!=null) return localArgValues.local(); 
      throw "key ["+name+"] not found in local scope";
   }
}
// getting "arguments.susi"
ArgScope {
   function get(name) {
      localArgValues=localarg.get(name);
      if(localArgValues!=null) return localArgValues.arg(); 
      throw "key ["+name+"] not found in arguments scope";
   }
}

Then we have 3 scopes pointing to the same Map holding the data, and the undefined scope has no longer to search 2 scope separatly, instead it can search one map.
Actually that solves 2 problems, it splits the search in half and makes it easier to handle “null”.
This is possible because the lifespan and reach of this 2 scopes is identical.

Thinking about this, that is a solution with all the benefits i’m looking for and no downside or backward compatibility issue i can think of. Thanks @pfreitag to point me in the right direction.

1 Like