node.getChild(string) vs DB.getChild(node, string)

[bmos]

I have recently heard in Discord that there are changes coming (https://discord.com/channels/274582899045695488/792625299878707210/1060255173785686056) around this (if I understand correctly).

In case it helps anyone, here are some regex replace strings I wrote up.

I used this to change node.getChild() to DB.getChild(node)

([\( ])([0-9A-Za-z]+[\(\)]*)(?<!DB)\.getChild\((.+)\)

$1DB.getChild($2, $3)

I didn't see any false positives in my code, but I would recommend only using regex replace on your code if you're using version control to vet all its changes.

I used this to change node.getChildren() to DB.getChildren(node)

([\( ])([0-9A-Za-z]+[\(\)]*)(?<!DB)\.getChildren\(\)

$1DB.getChildren($2)

Unfortunately, I haven't found a really good method of doing this for getValue, setValue, getType, and other functions that also exist for interface objects. These all result in many false positives (although it's still a time saver compared to changing everything by hand).

[bmos]

Also, does anyone have performance data around the two approaches?
Is DB.getChild faster or something?

[Moon Wizard]

I wasn't quite ready to move this change/concept forward yet, though it was pointed out to me that it was leaked on Discord that this is something we're working on in the background.

Currently, there is no expected positive or negative performance change between the two different approaches, because in order to keep everything working, both implementations have to be supported at this time.

The gist of the change is that the original FG developers (before my time) chose to make databasenode objects (as well as a few others) into full-fledged scriptable objects with embedded APIs, even though those objects can not be assigned a script tag/file. This creates a lot of overhead in every object, since the entire global namespace and APIs need to be built for every object. So, when you have an object that is as prolific as databasenode (i.e. every possible node in the database), my theory is that this creates a massive amount of overhead for database operations which is completely unnecessary. Also, with the continued growth of the FG catalog, the size of the databases used by DLC and in campaigns continues to grow with every year.

My working project is to migrate databasenode objects to simple Lua userdata objects without any additional script spaces, global namespaces or APIs to make them as streamlined as possible. However, since it is unknown how much of a performance savings this will gain, I had to build up the replacement APIs (which are useful anyway) and now I'm adjusting the CoreRPG and other included rulesets to only use the replacement APIs. This will allow me to make an internal build with the simplified databasenode data objects and actually run performance tests to see whether my theory is valid.

This is something I've been thinking about for many years, but the sheer volume of heavy lifting and disruption always pushed it to backburner. In reality, the coming changes are just one more step in the process that allows me to test the theory. It's not a high-priority project, so it gets done in small chunks. Once I have real testing numbers later this year, we will need to make a decision whether the performance gain justifies the short/medium term chaos of breaking DLC, or coming up with some migration methods.

Regards,
JPG

[Elawyn]

Will there be an easy explanation for Hobby, non commercial Coders like me to migrate the database node calls to DB. Value calls? F. E. I built an extension for Char sheet, npc sheet with automatization rolls etc and use database node calls for getchild, getparent get or Set value etc All over. Otherwise i hope for next Video from damned, who elevated my Project with this up to this point.

[damned]

Will there be an easy explanation for Hobby, non commercial Coders like me to migrate the database node calls to DB. Value calls? F. E. I built an extension for Char sheet, npc sheet with automatization rolls etc and use database node calls for getchild, getparent get or Set value etc All over. Otherwise i hope for next Video from damned, who elevated my Project with this up to this point.

Hi Elawyn

If/when Moon Wizard implements this we will first see DEPRECATION warnings in the console which will tell us what line the code is executing from. Then errors later on. Ultimately I think there will be a lot of Find in Files for "node" and then rewriting all the older method (which I use a lot :bandit: )
The required change is not difficult - it might be time consuming though!

[Elawyn]

Thx damned for Quick Response. I Was a little concerned :) so, i am looking forward to an explanation Video from you or a workaround contributed by moon wizard himself. Cheers!

[damned]

here are a couple of examples:

existing code:


local nodeWin = window.getDatabaseNode();
local sCommand = nodeWin.getChild("clichatcommand").getValue();
local sRollstype = nodeWin.getChild("rollstype").getValue();

local nStart,nEnd,sCommand,sParams = string.find(sCommand, '^/([^%s]+)%s*(.*)');
if sCommand == "rollon" then
nodeWin.getChild("rollstype").setValue("table");


this would need to be rewritten:

local nodeWin = window.getDatabaseNode();
local sCommand = DB.getChild(nodeWin, "clichatcommand").getValue();
local sRollstype = DB.getValue(nodeWin, "rollstype");

local nStart,nEnd,sCommand,sParams = string.find(sCommand, '^/([^%s]+)%s*(.*)');
if sCommand == "rollon" then
DB.getChild(nodeWin, "rollstype").setValue("table");


lines 2,3 and 7 have been changed.

where I have used getChild in conjunction with getValue I could have just used getValue but the examples should be valid (I hope).

EDIT in case people read this far and no further. Moon Wizard has said that those getChild getValues should also be rewritten at the same time:

Better Code

local nodeWin = window.getDatabaseNode();
local sCommand = DB.getValue(nodeWin, "clichatcommand");
local sRollstype = DB.getValue(nodeWin, "rollstype");

local nStart,nEnd,sCommand,sParams = string.find(sCommand, '^/([^%s]+)%s*(.*)');
if sCommand == "rollon" then
DB.setValue(nodeWin, "rollstype", "string", "table");

and then Trenloe adds that getParent() should also be replaced like this:


nodeWin.GetParent().GetParent().GetChild("string") .getValue("number")
4 API calls


DB.getValue(nodeWin, "...string.number", "")
is only 1 API call.

[Elawyn]

here are a couple of examples:

existing code:


local nodeWin = window.getDatabaseNode();
local sCommand = nodeWin.getChild("clichatcommand").getValue();
local sRollstype = nodeWin.getChild("rollstype").getValue();

local nStart,nEnd,sCommand,sParams = string.find(sCommand, '^/([^%s]+)%s*(.*)');
if sCommand == "rollon" then
nodeWin.getChild("rollstype").setValue("table");


this would need to be rewritten:

local nodeWin = window.getDatabaseNode();
local sCommand = DB.getChild(nodeWin, "clichatcommand").getValue();
local sRollstype = DB.getValue(nodeWin, "rollstype");

local nStart,nEnd,sCommand,sParams = string.find(sCommand, '^/([^%s]+)%s*(.*)');
if sCommand == "rollon" then
DB.getChild(nodeWin, "rollstype").setValue("table");


lines 2,3 and 7 have been changed.

where I have used getChild in conjunction with getValue I could have just used getValue but the examples should be valid (I hope).

Thank you. I know the db. Call til now only from the ctnode calls. But if thats the recommended change, i am looking forward to Do so. Thanks a lot!

[Moon Wizard]

To go one step further with @damned's example code:

Old Code


local nodeWin = window.getDatabaseNode();
local sCommand = nodeWin.getChild("clichatcommand").getValue();
local sRollstype = nodeWin.getChild("rollstype").getValue();

local nStart,nEnd,sCommand,sParams = string.find(sCommand, '^/([^%s]+)%s*(.*)');
if sCommand == "rollon" then
nodeWin.getChild("rollstype").setValue("table");


New Code


local nodeWin = window.getDatabaseNode();
local sCommand = DB.getValue(nodeWin, "clichatcommand");
local sRollstype = DB.getValue(nodeWin, "rollstype");

local nStart,nEnd,sCommand,sParams = string.find(sCommand, '^/([^%s]+)%s*(.*)');
if sCommand == "rollon" then
DB.setValue(nodeWin, "rollstype", "string", "table");


Lines 2 and 7 actually need to be even more streamlined.

Regards,
JPG

[Elawyn]

Even more, thank you too, mood wizard! If i understand it the right way, if i can get the value directly from db without specifing the child first, but taking the string in brackets (node, "string", "number" ). If starting a call from f. E. a list today i use nodeWin.GetParent().GetParent().GetChild("string").getValue("number"). Is this still neccassary/possible with the db. Call for the value? Ok. Sorry for explicit questions to you All. I will See and try, when the changes come in Action. But this is so far good to hear and Not so overwhelming like i was concerned about in first place.

[Trenloe]

...today i use nodeWin.GetParent().GetParent().GetChild("string").getValue("number").
Use relative DB paths - info on DB paths here: https://fantasygroundsunity.atlassian.net/wiki/spaces/FGCP/pages/996644468/Ruleset+-+Database#Database-Paths

So, you could use DB.getValue(nodeWin, "...string.number", "") would go two steps up in the DB hierarchy from nodeWin (the first "." is the same location as nodeWin) and then proceeds to the child string and from there returns the value of the child field called "number".

[Elawyn]

Thx a lot, will that try back at home!

[seansps]

The gist of the change is that the original FG developers (before my time) chose to make databasenode objects (as well as a few others) into full-fledged scriptable objects with embedded APIs, even though those objects can not be assigned a script tag/file. This creates a lot of overhead in every object, since the entire global namespace and APIs need to be built for every object. So, when you have an object that is as prolific as databasenode (i.e. every possible node in the database), my theory is that this creates a massive amount of overhead for database operations which is completely unnecessary. Also, with the continued growth of the FG catalog, the size of the databases used by DLC and in campaigns continues to grow with every year.

Hi Moon Wizard! This is a great theory and I think you are spot on here. I see a lot of slowness currently when using the API to copy large DB lists of DB nodes and children around, and perhaps this will be a great speed improvement. I have a few concerns though. Namely, there are a lot of unofficial rulesets (on the Forge) etc. that are probably not aware of such a change, and this would definitely break all of those rulesets. Additionally, we have a release coming up for Cyberpunk RED, which is coming along nicely, but we will need to do a lot of refactoring (immediately after release preferably) and then a full regression test, to ensure issues do not crop up as a result. It is a lot of work (and we are not full SW employees).

If what I understand here is correct, each dbnode reference (a userdata object) is loading dependencies needed to be able to do those API calls, probably lots of XML libraries and such. Now, I am not a Lua expert by any means, but perhaps there is another way to do this refactor while ensuring cross-compatibility with older rulesets.

Would something like this be possible?

Keep dbnode references a userdata object with .getChild, .getChildren, .getValue, and .setValue
Refactor the underlying userdata object (C++ I am assuming? C#?) to instead call a singleton that is managed by the application. The singleton is then responsible for loading all the XML packages/libraries needed, and then calls those needed for each call, with the values as specified by the userdata object.


The idea here is then just one singleton class/userdata object is instantiated for the whole application that is responsible for doing XML lookups, setting values, etc., and each dbnode just points to that singleton wrapper.

Again, apologies if this was already considered or is just not possible, I have never edited an application's Lua script engine, and my knowledge of the Lua interpreter is pretty limited.

[Moon Wizard]

On the @Trenloe/@Elawyn discussion, one note is that the new method will be much more performant than the concatenation method, because you have less API calls that have to be translated between Lua to the core engine.

nodeWin.GetParent().GetParent().GetChild("string").getValue("number") = 4 API calls

DB.getValue(nodeWin, "...string.number", "") = 1 API call.

Regards,
JPG

[Moon Wizard]

@seansps,

API references are actually tied to directly to each individual object, so that there is context for when an API is called. If it was a general function, there would be no context.

Also, all the APIs have to be registered to each object in order to be linked to an object in order for Lua to work which is part of the overhead.

I’ll definitely be looking at ways to possibly minimize disruption once I can verify whether the theory that performance will be affected is tested with a special client with the databasenode rewritten.

Thanks,
JPG

[Elawyn]

Thank you and yes, it seems "smoother" and i hope your work will be fruitful! I tested the db. Method for string calls and number calls, both are working (for me such Things are exiting, sorry :)) I am relieved so far. My nodeWin.getPath() i have struggle with but i will think about and if i unterstand the timeline right, its Not an instant encounter.

[Moon Wizard]

Most single-call functions can usually be rewritten pretty easily nodeVar.fn(...) -> DB.fn(nodeVar, ...)

The exception is DB.setValue which requires a type parameter, and chained calls which end up having to be nested.
Also, most chained calls are of the form nodeVar.getParent().getParent()... or nodeVar.getChild(...).getChild(...). These are faster to implement as paths using the dots for going up/down tree.

Regards,
JPG

[seansps]

@seansps,

API references are actually tied to directly to each individual object, so that there is context for when an API is called. If it was a general function, there would be no context.

Also, all the APIs have to be registered to each object in order to be linked to an object in order for Lua to work which is part of the overhead.

I’ll definitely be looking at ways to possibly minimize disruption once I can verify whether the theory that performance will be affected is tested with a special client with the databasenode rewritten.

Thanks,
JPG

Thanks for the response! That’s too bad if it can’t work with a singleton…

I believe that when multiple instances of a userdata object are created in a Lua script, with pointers to a singleton, then they’d all just reference the same underlying object in memory. If the dbnode objects pointed to a singleton object it should only consume memory once, regardless of how many references to it exist in the Lua script (at least for the API portion.)

But that’s just my theory. I’ve never implemented anything like this.

[damned]

How would you rewrite the second line here?


local nodeRoll = window.getDatabaseNode();
local nodeChar = nodeRoll.getParent().getParent();

[superteddy57]

local nodeChar = DB.getChild(nodeRoll, "...");

[damned]

Thanks superteddy57

[bmos]

What about nested createChild calls?

DB.createChild(DB.createChild(nodeSpellLevel, 'spells'))
Is there a way to do something like this with one API call?

[bmos]

Also, here are two more string replacements I found useful


DB.getName\(DB.getParent\((.+)\)\)

DB.getName($1, '..')

and


DB.getName\(DB.getChild\((.+), '(.+?)'\)\)

DB.getName($1, '$2')

[Moon Wizard]

There's not going to be a way to be able to simplify this call any further, unless you are able to specify the inner child name completely (which doesn't seem like the right call for this example).


DB.createChild(DB.createChild(nodeSpellLevel, 'spells'))


Regards,
JPG

[SilentRuin]

I need clarification on this for when the Current TEST goes live - are these changes something we HAVE to do before it goes live or just some current thing SW is testing to see if its worth doing? As in do I have to rewrite everything to do these things this round or can I wait?

[Moon Wizard]

You can currently wait, as I'm still in the phase of getting everything set up to assess.

However, in general, the DB functions are more concise, so should be preferred long-term.

Regards,
JPG

[bmos]

I need clarification on this for when the Current TEST goes live - are these changes something we HAVE to do before it goes live or just some current thing SW is testing to see if its worth doing? As in do I have to rewrite everything to do these things this round or can I wait?To clarify Moon Wizard's response:
Only the rulesets need these changes currently. Once the rulesets are coded this way, Moon Wizard is going to make a private build of FGU that does not embed those functions in databasenode objects. This will allow him to verify his theory that it's responsible for some of the performance issues.
Extensions will not be part of that testing, so their use of the embedded functions does not need to change. If the testing says it's worth pursuing, then the embedded functions will be officially deprecated (hopefully via a gradual process as has been done with other recent deprecations) or re-implemented to address the bottlenecks. This is when extension devs would really need to update their code.

That being said, I've already started doing it because I maintain like 2 dozen extensions and I'd rather chip away on it over months than have to rush through it ahead of some breaking update.

[bmos]

@Moon Wizard
Do your concerns also extend to the embedded functions in other types of objects?
I'm thinking of things like sString:gsub(pattern, replace) vs string.gsub(sString, pattern, replace)

[seansps]

To clarify Moon Wizard's response:
Only the rulesets need these changes currently. Once the rulesets are coded this way, Moon Wizard is going to make a private build of FGU that does not embed those functions in databasenode objects. This will allow him to verify his theory that it's responsible for some of the performance issues.
Extensions will not be part of that testing, so their use of the embedded functions does not need to change. If the testing says it's worth pursuing, then the embedded functions will be officially deprecated (hopefully via a gradual process as has been done with other recent deprecations) or re-implemented to address the bottlenecks. This is when extension devs would really need to update their code.

That being said, I've already started doing it because I maintain like 2 dozen extensions and I'd rather chip away on it over months than have to rush through it ahead of some breaking update.

I'd really like to push back on deprecating any API currently attached to any `databasenode` userdata object.

Even a gradual deprecation process would have to be spread out over the course of years (not months). Maybe 3 or 4. This would not only impact rulesets, it would impact every single extension ever made for FGU that also leverages this API. As a ruleset dev myself, it is a huge refactor (as we do use that API quite a bit) with an extensive regression test process to make sure we don't slip and introduce any bugs.

If what I am suggesting previously is possible, and I do believe it is, a better refactor can be done that allows the `databasenode` userdata object to access an API exposed by one object maintained in memory, thus achieving the same, or at least nearly similar, level of optimization.

Also, if I am understanding Moon Wizard correctly, it seems that they may have found a way to do something like that, so that a total refactor is not needed (though may still yield a performance boost.)

I just want to state I (respectfully) disagree with deprecating these functions, I think it will be a huge mistake for the community.

[MeAndUnique]

@Moon Wizard
Do your concerns also extend to the embedded functions in other types of objects?
I'm thinking of things like sString:gsub(pattern, replace) vs string.gsub(sString, pattern, replace)

The example syntax here is actually 100% identical between the two under the hood in Lua. Arguably, leveraging this language feature could actually be a viable middle ground for database nodes, where they could support ":" notation that routes to DB API calls.

[MeAndUnique]

I'd really like to push back on deprecating any API currently attached to any `databasenode` userdata object.

Even a gradual deprecation process would have to be spread out over the course of years (not months). Maybe 3 or 4. This would not only impact rulesets, it would impact every single extension ever made for FGU that also leverages this API. As a ruleset dev myself, it is a huge refactor (as we do use that API quite a bit) with an extensive regression test process to make sure we don't slip and introduce any bugs.

If what I am suggesting previously is possible, and I do believe it is, a better refactor can be done that allows the `databasenode` userdata object to access an API exposed by one object maintained in memory, thus achieving the same, or at least nearly similar, level of optimization.

Also, if I am understanding Moon Wizard correctly, it seems that they may have found a way to do something like that, so that a total refactor is not needed (though may still yield a performance boost.)

I just want to state I (respectfully) disagree with deprecating these functions, I think it will be a huge mistake for the community.

While I am very sympathetic with the concerns, if indeed it turns out to be a system problem with databasenodes degrading performance, the best thing for the community is in fact to bite the bullet and deprecate the problematic functionality. While worth avoiding wherever possible, this is a common reality for the maturation of any coding framework and necessary to retaining long-term viability. Performance of FGU is already a notable pain-point for many users and use-cases, and one that if left untended will inevitably result in FGU becoming noncompetitive in the VTT arena.

[Moon Wizard]

As MeAndUnique mentioned, the syntax for the strings is identical under the hood for Lua, but only for functions tied to a variable type. [vartype.fn(var, ...) == var:fn(...)]

Unfortunately, the whole point of this process is to make the databasenodes into very simple objects with no functions tied to them. If we don't have to register an entire global space to them, then that is a lot of overhead saved multiplied by the number of database nodes (hundreds of thousands or more per campaign with modules loaded). So, I can't just "hide" them in another calling method, or you still have all the overhead.

To be clear, if the performance is sufficient, this will be a planned under a year process with migration needed.
The migration is almost all a very straight regex replacement except for possibly nested calls, which are slow performance by themselves (i.e. multiple calls vs. single call).

Regards,
JPG

[MeAndUnique]

If what I am suggesting previously is possible, and I do believe it is, a better refactor can be done that allows the `databasenode` userdata object to access an API exposed by one object maintained in memory, thus achieving the same, or at least nearly similar, level of optimization.

Also, if I am understanding Moon Wizard correctly, it seems that they may have found a way to do something like that, so that a total refactor is not needed (though may still yield a performance boost.)

Though to clarify, I wholly agree with the concept here if it is in anyway feasible. Taking it even a step further, any mechanism afforded by the Lua library FGU uses (or even one of the alternatives, being several candidates) that more closely aligns the object and memory management will itself yield numerous benefits to performance overall. As a simple example, the recent addition of a getChildList as an alternative to getChildren, if mapped to C# Dictionary-like synonyms would yield performance measures that differ only by passing a single additional reference, or in other words a handful of nanoseconds per loop. So considering the entire DB would still be way under a single second difference, complete noise in light of other performance concerns.

[seansps]

The example syntax here is actually 100% identical between the two under the hood in Lua. Arguably, leveraging this language feature could actually be a viable middle ground for database nodes, where they could support ":" notation that routes to DB API calls.

This is something I've considered as well.

It is also possible to achieve this using `.` notation if the underlying object is a Lua table. I wrote some C# code as a proof of concept that works when running locally.

[seansps]

Though to clarify, I wholly agree with the concept here if it is in anyway feasible. Taking it even a step further, any mechanism afforded by the Lua library FGU uses (or even one of the alternatives, being several candidates) that more closely aligns the object and memory management will itself yield numerous benefits to performance overall. As a simple example, the recent addition of a getChildList as an alternative to getChildren, if mapped to C# Dictionary-like synonyms would yield performance measures that differ only by passing a single additional reference, or in other words a handful of nanoseconds per loop. So considering the entire DB would still be way under a single second difference, complete noise in light of other performance concerns.

Yes, exactly.



var lua = new Lua();

// Expose the databasenode userdata object to Lua
var databasenode = new DatabaseNode();

// Create a Dictionary that wraps the databasenode methods
var databasenodeWithMethods = new Dictionary<string, Delegate>();
databasenodeWithMethods["getValue"] = () => {
databasenode.getValue();
};
databasenodeWithMethods["setValue"] = (string val) => {
databasenode.setValue(val);
};
databasenodeWithMethods["getPath"] = () => {
databasenode.getPath();
};
lua["databasenode"] = databasenodeWithMethods;


The above code uses the NLua C# library. The `DatabaseNode` is a class that just has a string pointing to a XML file and a pointer to an XML element.
The method calls on the `DatabaseNode` use an `XmlWrapper` class powered by C# `System.XML` and it each `DatabaseNode` just gets a pointer to the single instance maintained by the application using `xmlWrapper = XmlWrapper.Instance` which is one instance. This should achieve only one instance of the underlying XML Library needed by the application without having each database node instance instantiate their own.

I can provide more of what I did locally as a POC if it helps.

Edit: And yes in the above I was just printing within the methods instead of returning the values needed, but it's possible to wire it up properly.

[Moon Wizard]

On the string.fn(var, ...) vs. var:fn(...) part of the discussion; this approach with databasenode objects would not smooth migration any more than migrating to DB. That is because all the var.fn(...) calls with databasenode vars would need to be converted to either databasenode.fn(var, ...) or var:fn(...). Not really much savings in coding effort in that approach.

The current recommended change is DB.fn(var, ...). How is that different than the former conversion above?

Also, I think that the level of work that is being assumed with this change is higher than it is, I was able to convert about 20 rulesets in a couple days, including Core which is larger than most of the other rulesets.

Regards,
JPG

[seansps]

On the string.fn(var, ...) vs. var:fn(...) part of the discussion; this approach with databasenode objects would not smooth migration any more than migrating to DB. That is because all the var.fn(...) calls with databasenode vars would need to be converted to either databasenode.fn(var, ...) or var:fn(...). Not really much savings in coding effort in that approach.

The current recommended change is DB.fn(var, ...). How is that different than the former conversion above?

Also, I think that the level of work that is being assumed with this change is higher than it is, I was able to convert about 20 rulesets in a couple days, including Core which is larger than most of the other rulesets.

Regards,
JPG


It’s not just converting the code, but also, the amount of time needed to do a full regression test of a ruleset to ensure no bugs get into production.

Additionally, all the extensions that would need conversion as well.

In any case, check my response above — there’s a way to maintain “.” Notation functionality and not embed the API into each userdata object.

[Moon Wizard]

Actually, there is not a way to maintain a dot notation in Lua without embedding the API into each object. The way that Lua works is that the dot notation is a Lua table lookup, so that name = fn entry must exist in the object in order for var.fn(...) to work.

As mentioned above, adding a "databasenode" "library" to support databasenode.fn(var, ...) or var:fn(...), is not that different than DB.fn(var, ...). The example you displayed above just creates a "databasenode library".

Regards,
JPG

[seansps]

Actually, there is not a way to maintain a dot notation in Lua without embedding the API into each object. The way that Lua works is that the dot notation is a Lua table lookup, so that name = fn entry must exist in the object in order for var.fn(...) to work.

As mentioned above, adding a "databasenode" "library" to support databasenode.fn(var, ...) or var:fn(...), is not that different than DB.fn(var, ...). The example you displayed above just creates a "databasenode library".

Regards,
JPG

I see, how about this?


using System;
using System.Xml;
using NLua;

class Program
{
static void Main(string[] args)
{
var lua = new Lua();

// Expose the databasenode userdata object to Lua
var databasenode = new DatabaseNode();

lua["databasenode"] = databasenode;

// Run the Lua script
lua.DoFile("script.lua");
}
}

class DatabaseNode
{
private XmlWrapper xmlWrapper;
private XmlNode nodeElement;

public DatabaseNode()
{
xmlWrapper = XmlWrapper.Instance;
var xmlDoc = xmlWrapper.LoadXmlFile("database.xml");
var children = xmlDoc.GetElementsByTagName("charsheet");
nodeElement = children.Count > 0 ? children.Item(0) : null;
getValue = this.GetValue;
setValue = this.SetValue;
getPath = this.GetPath;
}

public Delegate getValue;
public Delegate setValue;
public Delegate getPath;

public string GetValue()
{
return nodeElement.InnerText;
}

public string SetValue(string value)
{
// TODO
return "SET TO " + value; ;
}

public string GetPath()
{
return "database.xml";
}

public void GetChild()
{
Console.WriteLine("TODO");
}

public void GetChildren()
{
Console.WriteLine("TODO");
}

/// ... etc, other methods needed for DB Node
}

That should create a databasenode userdata object with properties that call the underlying instance methods. It uses a singleton to get to the XML Library.

[Moon Wizard]

You're still assigning this library of functions to "databasenode" as a variable. It's all the same thing you're hashing over.

Regards,
JPG

[seansps]

You're still assigning this library of functions to "databasenode" as a variable. It's all the same thing you're hashing over.

Regards,
JPG

Right, this is just for a POC, as it's just a basic terminal app.

The code that returns userdata objects for `DatabaseNode`s (like window.getDatabaseNode()) would need to be modified to instantiate a reference to this new class.

[MeAndUnique]

On the string.fn(var, ...) vs. var:fn(...) part of the discussion; this approach with databasenode objects would not smooth migration any more than migrating to DB. That is because all the var.fn(...) calls with databasenode vars would need to be converted to either databasenode.fn(var, ...) or var:fn(...). Not really much savings in coding effort in that approach.

The current recommended change is DB.fn(var, ...). How is that different than the former conversion above?

Also, I think that the level of work that is being assumed with this change is higher than it is, I was able to convert about 20 rulesets in a couple days, including Core which is larger than most of the other rulesets.

Regards,
JPG

Strictly speaking the syntax I was hypothesizing would be converting the existing "node.function(....)" to "node:function(...)" where database nodes are treated similarly to string under the hood and mapped to DB API calls.

[MeAndUnique]

Actually, there is not a way to maintain a dot notation in Lua without embedding the API into each object. The way that Lua works is that the dot notation is a Lua table lookup, so that name = fn entry must exist in the object in order for var.fn(...) to work.

As mentioned above, adding a "databasenode" "library" to support databasenode.fn(var, ...) or var:fn(...), is not that different than DB.fn(var, ...). The example you displayed above just creates a "databasenode library".

Regards,
JPG

The cost in performance isn't in the act of being a table, it is in the act of creating a table. If the database node tables are created once and reused, then injecting a couple dozen function references once at instantiation (can even be done lazily) is genuinely negligible (massively less than 1%) in a system that uses ZIP archives as databases. Especially when considering that the FGU application itself already has to load some form of object for each node anyway to parse it from the underlying XML, which means the Lua userdata mechanism for mapping the underlying data could point directly to the C# objects resulting in script-side node interactions having virtually zero overhead of any sort.

[MeAndUnique]

Right, this is just for a POC, as it's just a basic terminal app.

The code that returns userdata objects for `DatabaseNode`s (like window.getDatabaseNode()) would need to be modified to instantiate a reference to this new class.

The omission of the salient detail of plugging into existing APIs is I think the crux of the contention here, as the example of registering a global databasenode object to the Lua context is in direct contradiction to the desired result.

[seansps]

The omission of the salient detail of plugging into existing APIs is I think the crux of the contention here, as the example of registering a global databasenode object to the Lua context is in direct contradiction to the desired result.

I am not sure I understand what you're saying here.

Basically, any time a databasenode is returned (a userdata object) by a call to a control or some other userdata object, the underlying C# application would instantiate a new `UserData` class and return that. Each class does not have a whole XML library instantiated/loaded in it (as I understand the current problem is) because it points to a reference managed by a Singleton, of which the application would only instantiate one of.

[Moon Wizard]

But all the functions have to be added to every databasenode object, or you can't call it like this [var.fn(...)]. Adding an additional "databasenode" context in the global space does not address that at all.

Regards,
JPG

[seansps]

But all the functions have to be added to every databasenode object, or you can't call it like this [var.fn(...)]. Adding an additional "databasenode" context in the global space does not address that at all.

Regards,
JPG

I am sure I am missing a lot of context (since I don't have the sourcecode) but wouldn't the every databasenode object have all the functions, if instantiated using the example code I provided? This wouldn't be a lot of overhead, because the class itself only has a reference to an XML library, and doesn't create the memory for the library itself.

[Moon Wizard]

As you say, I think there is context that you are missing. While I'm sure there will be more investigation into this part of the code when I get there, I think you are missing some key information in the way that Lua works in the interaction. A global object/script is very different than a custom data type with custom APIs.

For databasenodes to be called like they do know, you have to define every fn_name -> fn definition for every object in order to even allow that syntax to work, and then you have to make it contextual so that you know which object the function should be applied to (currently FG applies the fn_name -> object specific fn instance).

Regards,
JPG

[seansps]

As you say, I think there is context that you are missing. While I'm sure there will be more investigation into this part of the code when I get there, I think you are missing some key information in the way that Lua works in the interaction. A global object/script is very different than a custom data type with custom APIs.

For databasenodes to be called like they do know, you have to define every fn_name -> fn definition for every object in order to even allow that syntax to work, and then you have to make it contextual so that you know which object the function should be applied to (currently FG applies the fn_name -> object specific fn instance).

Regards,
JPG

Ahh okay, interesting. Thanks for the additional context. Sounds like quite the challenge in terms of tech debt. Hopefully a good compromise can be found!

[Moon Wizard]

Unfortunately, the tech debt was acquired before my involvement with FG, and part of the original API design in FG v2. I would like to move all custom data types to a model like the databasenode -> DB migration I'm proposing and working on.

While I know that there are savings to be add in terms of computing effort/speed with the change, the big question is still what the magnitude of the changes are. I'm hoping for a large change that will make everyone excited; but as MeAndUnique mentioned, I need to test and figure out if the gains rise above the level of a margin of error in the processing times.

Regards,
JPG

[seansps]

Sounds good! And no worries, I’m not trying to point blame with regards to the tech debt. Every organization, every codebase, has some — it’s always a challenge to clean it up. If it means big performance boosts to FGU, it’s good to do.

[damned]

For me the replacement of the code is one task - Im sweating on fixing all the errors Ill make!

[Moon Wizard]

I think out of about 20 rulesets, there was only like 3 issues I found so far. I think it was mostly unwinding chained calls into a single call that tripp e me up on those. The straight replacements were easy; and could be regex’d.

JPG

[damned]

Do you have some regex examples to share?

[seansps]

Curious as well. A regex find/replace won’t be able to to determine if using “.getValue” on say a numberfield Vs a database node object.

[superteddy57]

Well the in house standard is to name node variables with a preceding node prefix (ex: nodeChar). I will wait for Moon to provide his examples, but using that prefix did help in replacing many of the ones I needed to replace.

[seansps]

Well the in house standard is to name node variables with a preceding node prefix (ex: nodeChar). I will wait for Moon to provide his examples, but using that prefix did help in replacing many of the ones I needed to replace.

True- but while I do try to keep to that… I’m sure there are places that aren’t that way.

[superteddy57]

I understand. Just offering a bit of help in locating those lines that need changing.

[seansps]

I understand. Just offering a bit of help in locating those lines that need changing.

Thanks! Yeah, I am thinking if I can get a regex to do most of the replacements (accounting for `[nN]ode\w*.` and `\w*[nN]ode`) then perhaps that will cover most and the rest can be a manageable amount to eyeball.
SO much to test though, after such a change. I will definitely be holding off until we know more about the performance gains.

[superteddy57]

Another tip was as I going through, I would only change a section and then test that section out. Like if I'm going through the ship creation code, then I go create a ship. It's a bit slow, but gives you piece of mind that doesn't require back tracking. This may sound common advice for any debugging, but more so with this as the console will yell if something is amiss and you are all ready in that section of code.

[Moon Wizard]

To be clear, I used the regex mostly to identify what needed to be changed, and didn't use them as a global replacement. I typically just stepped through with an automatic replacement for easy ones and skipped the ones it wouldn't work for. Then, I searched for the remaining ones, and fixed manually.

To identify outstanding fixes to be made, the regex I used was:
[^B]\.getChild\(
(apply once for each function; see below)

To do simple replacement search/replace (using Replace one at a time, not Replace All):
(node[A-Za-z]*)\.getChild\(
DB.getChild(\1,
(apply once for each function; see below)

Functions to check:
getName
getValue, getText, setValue,
getParent, createChild, getChild, getChildren, getChildList, getChildCount, getChildrenGlobal,
isEmpty,
getPath, getNodeName, getModule, getType, delete,
getVersion, getRulesetVersion, updateVersion,
getHolders, addHolder, removeHolder, removeAllHolders, getOwner, setOwner, isOwner,
isReadOnly, setStatic, isStatic, setPublic, isPublic,
isIntact, revert,
getCategory, setCategory, getChildCategories, updateChildCategory, addChildCategory, removeChildCategory, getDefaultChildCategory, setDefaultChildCategory,

I also migrated all the databasenode handlers to DB.addHandler/DB.removeHandler as well. Plus, the handlers are much cleaner the new way, since you don't have to assign empty functions, and leave them always assigned.

Handlers to check:
onUpdate, onDelete,
onChildAdded, onChildDeleted, onChildUpdate,
onObserverUpdate, onIntegrityChange, onCategoryChange, onChildCategoriesChange,


Regards,
JPG

[seansps]

Any updates on the DB Node Migration experiment? Is this something we should all start doing in our rulesets? I got quite a bit of work ahead of me if so!

[Moon Wizard]

No, several other projects are in the queue; so I haven't been able to circle back and do the testing I want to verify whether the changes provide the speed-ups I expect or not.

Regards,
JPG

[seansps]

No, several other projects are in the queue; so I haven't been able to circle back and do the testing I want to verify whether the changes provide the speed-ups I expect or not.

Regards,
JPG

Sounds good, thanks for the update!