hydrobyte / McJSON

A Delphi / Lazarus / C++Builder simple and small class for fast JSON parsing.
MIT License
58 stars 20 forks source link

Slower shortener operation (but I realized this is normal because of the AddOrSet search) #20

Closed totyaxy closed 1 year ago

totyaxy commented 1 year ago

Hi

I started to standardize the code from the traditional code to a shortener version (which had not yet been rewritten). The code below in the shortened version is approx. 20 times slower. It is possible that this is not how the use of the shortener code was invented (create object), but it makes sense to me and it works: ( "->" just an indicator in the code)

var
 Json: TMcJsonItem = nil;
 JsonDataObject: TMcJsonItem = nil;
 JsonIDObject: TMcJsonItem = nil;

begin
...
Json := TMcJsonItem.Create;
...
      JsonDataObject := Json.O[cJsonDataObject]; // Create object... ('JsonData')

      for i := 0 to Self.Count - 1 do
      begin
        IndexStr := IntToStr(Self.Data[i].Index);
        -> JsonIDObject := JsonDataObject.O[IndexStr]; // Create object... this 20 times slower than the next line:
        -> JsonIDObject := JsonDataObject.Add(IndexStr); // Create object... (I know it isn't a correct code, but see ***

        for lt in TLanguageTypes do
          JsonIDObject.Add(lt.KeyName).AsString := Self.Data[i].GetItem(lt);
      end;

What I also don't understand is that earlier you showed a code for creating an empty key (object) https://github.com/hydrobyte/McJSON/issues/17#issuecomment-1468314957:

So the correct code for to create object, this (non shortener variant): ***

->JsonIDObject := JsonDataObject.Add(IndexStr, jitObject);

But no, because the compiler gives an error:

Error: Incompatible type for arg no. 2: Got "TJSONInstanceType", expected "TJItemType"

but the "jitObject" is TJItemType:

TJItemType  = (jitUnset, jitValue, jitObject, jitArray);

Thank you for your patience! I tried to describe everything as accurately as possible.

totyaxy commented 1 year ago

In the meantime, I realized that I started using shortener because it also implements the addorset function mentioned earlier, which can simplify the code. So the shortener is not just simply a short version, it also has a different operational function. It follows, however, that in the case of a new key/object, it is necessary to check whether the key/object exists, which takes a lot of time with a large number of keys/objects. Maybe with the global "speed" parameter it would be possible to set it to just add instead of shortener addorset?

But regardless of all this, the most important question is why the code you wrote, specifically for add objects, does not work, why the compiler gives an error (as I wrote in the previous post):

JsonIDObject := JsonDataObject.Add(IndexStr, jitObject);

Thank you very much!

totyaxy commented 1 year ago

Ok, the compiler error doesn't seem to come from MCJson, because jitObject is also found in fpJSON, and that caused the compiler error. By the way, I kept fpJSON because of the faster escape. I made an interface unit to avoid fpJSON, and thus there are no more compiler errors! And I immediately found an error in the escape function: #21

However, what I suggested in the previous comment, that the Add and AddOrSet functions could be selected in two for the shortener in some way (like Check/aSpeedUp), still exists. Thank you!

hydrobyte commented 1 year ago

Hi,

These kind of examples do not help me to reproduce the error.

Try to create examples outside your current work, something that I can esealy reproduce.

It follows, however, that in the case of a new key/object, it is necessary to check whether the key/object exists, which takes a lot of time with a large number of keys/objects. Maybe with the global "speed" parameter it would be possible to set it to just add instead of shortener addorset?

It will not work like speed parameter because with TList we have to use linear search to find a object using its key. The speed parameter works when creating, reading or parsing a JSON object, in order to check (or not) for key duplicity. It won't work with HasKey search function.

However, what I suggested in the previous comment, that the Add and AddOrSet functions could be selected in two for the shortener in some way (like Check/aSpeedUp), still exists. Thank you!

It is not the case. Use shorteners for a AddOrSet functionality or HasKey and Add as an indirect function.

I'll close this.

If you want, open another issue with cleaner examples.

Best.

totyaxy commented 1 year ago

If you want, open another issue with cleaner examples

Not needed, if fpJSON in the "uses" line, this line compile failed:

JsonIDObject := JsonDataObject.Add(IndexStr, jitObject);
hydrobyte commented 1 year ago

Hi,

I was thinking about this comment:

The code below in the shortened version is approx. 20 times slower.

I've estimated that the compile error was not related to McJSON.

totyaxy commented 1 year ago

Hi!

I understand why it's slower, because the shortener "add" is not just plain "add", but "add or set", so it checks if the given key exists, and this is time-consuming for many keys, similar to #19. That's why I had the idea that the shortener add and addorset functions could be selected separately with, say, a global variable, because in this way the shortener version does not replace the normal version, since it works differently. But if you don't understand what I'm talking about, if I have time I'll make a sample program (but I think it's unnecessary, now I'm asking you to read what I wrote more carefully).

hydrobyte commented 1 year ago

Hi,

I think I got it, but I also think the "add or set" with a external flag might not be necessary.

Note: I'm completely against global flags for setting up functionalities. It is very error prone.

So, let me show you some examples, that depends what you need:

  1. Just add, without checking (for key duplication): M.Add('key').AsString := 'value';
  2. Add checking that there is a key already: if (not M.HasKey('key')) then M.Add('key').AsString := 'value';
  3. Like example 2, if you use shortener M.S['key'] := 'value' it will try to find key. If found, change its value. If not, add it.

So, it is expected that methods 2 and 3 are time consuming due to the direct (2) or indirect (3) call of HasKey.

I think you do not need flags here. If you want the fastest case possible, use example 1.