Circular json reference wasnt completaly resolved

[gtreskas]

Hi,

Based on the changes here #94 still the issue persists when you use SmartLink control. The complete Properties panel dissapear in case of sap.ui.comp.navpopover.SmartLink.
I resolved the issue with circular dependencies by adjusting _prepareMessage method, setting the current.target[key] = null in case of circular reference is found. I dont understand why we should use an existing dependency in the first place --> This may be the root of the new circular reference, because it may re-introduce a circular dependency. And anyway you dont really care for the information so deep into the JSON.

Once I did the above, the circular dependency disappeared, but I was playing around with a Fiori Application Smart Template List Report and the performance became a disaster. The _prepareMessage is very slow and makes the whole experience very bad. I would suggest adding an extra parameter called "level" (like the node.js util.inspect method) where you can determine in what kind of depth should the reading of properties stop. I think is a waste of time reading the complete depth, especially in aggregation, association object, where the depth can become very deep to handle.

Let me know if I can help you with something!

[SeeSharpSoft]

@gtreskas As the author of the referenced fix I'm genuinly baffled about basically every sentence I just read here 😕

Based on the changes here #94 still the issue persists when you use SmartLink control. The complete Properties panel dissapear in case of sap.ui.comp.navpopover.SmartLink.

No, this is a different issue. It happens while executing port.postMessage(detectEvent.detail). Circular references are handled by its structured clone algorithm. Therefore having those is not an issue here - except there is actually a bug in this algorithm (sounds quite unlikely, but it is still human written code after all).

I resolved the issue with circular dependencies references by adjusting _prepareMessage method, setting the current.target[key] = null in case of circular reference is found

This portion of code does not find/handle circular references explicitely, it just re-uses already parsed references - circular or not. And with this, it also resolves circular references properly. So you did not "resolve the issue", you introduced a bug by altering the result.

I dont understand why we should use an existing dependency references in the first place --> This may be the root of the new circular reference, because it may re-introduce a circular dependency reference.

Cause the aim is on purpose to keep the data as it is, not getting rid of any references. Resolving circular dependencies references does not mean removing them but handle them properly while removing methods from the structure (which is btw the one and only reason why the message is parsed in the first place).

Once I did the above, the circular dependency disappeared, but I was playing around with a Fiori Application Smart Template List Report and the performance became a disaster. The _prepareMessage is very slow and makes the whole experience very bad.

Some simple measurements confirm my suspicion: The _prepareMessage implementation is actually faster than the previous used JSON.parse(JSON.stringify(object)) coding. Do you have other results?

The only "disaster" in performance I did notice, is when clicking on a sap.ui.comp.navpopover.SmartLink node in the control-tree and its message is parsed async in background. I don't know what is wrong with the sap.ui.comp.navpopover.SmartLink control, but the retrieved data for it is HUGE (which is still not the root cause here but responsible for the bad perceived performance when further navigating within the control tree).

Of course, this is performance wise worse than a directly failing JSON.stringify. I really hope nobody is surprised here: Properly parsing an object is slower than not parsing an object at all. (Konfuzius) 🙈

I would suggest adding an extra parameter called "level" (like the node.js util.inspect method) where you can determine in what kind of depth should the reading of properties stop. I think is a waste of time reading the complete depth, especially in aggregation, association object, where the depth can become very deep to handle.

The data that is parsed is not only the control/model information, but also e.g. the control tree. For each hierarchy level, the datastructure requires 2 "levels" within the provided message (plus some additional fixed structural overhead). So a control tree of e.g. depth 20 requires a minimum(!) of "level" 40 to be parsed in _prepareMessage. The issue with sap.ui.comp.navpopover.SmartLink happens already with a max-level of 6 (in words: "six").

Even though an "infinite" depth parsing of the control- or model-data is rarely (to never) required, it would not resolve this particular issue, and would furthermore only be useful in the same amount of rare (to none) cases. And it could cause missing information e.g. in the control tree.

Let me know if I can help you with something!

Lets find the actual root cause, then we can make informed decisions based on it.

And for that, I would start at where the issue actually occurs: The postMessage method of the chrome.runtime Port - what are its limitations, what are potential faulty inputs, what are known issues, checking bug reports etc.

And from there, check the actual input of the call in case serializing sap.ui.comp.navpopover.SmartLink control information. As mentioned, the input can be stripped down to depth level 6 in the _prepareMessage method.

THE END

[gtreskas]

@SeeSharpSoft : Thanks for the detail explanation, well it may be I was working too late yesterday and I miss to state the facts in the right way, sorry don't get offended or something from the issue here, I m sure you and your team are doing great job here.

So lets just go to the concrete case and try to reproduce the issue and I can show you on my screen how you can reproduce the issue or setup a call and have a look together.

The actual root cause is a circular reference and there was a comment on your code,, which was stating that you are handling it through copying an existing structure, but you are right, maybe I missunderstood your code. Currently I build a small fallback method in my fork which doesnt resolve the problem directly, but at least it works for now.

Let me know concretaly how to proceed?

Best Regards

[SeeSharpSoft]

@gtreskas Just to clarify: I am a returning, but still individual contributor to this project. Not part of the team, not having any more saying in this than you.

My lengthy response is about making informed decisions based on a proper root cause analysis, instead of trying to patch its effects. Those patches often causes additional issues that are then patched again etc. I am an advocate of the saying: "If you have one hour to solve a problem, then spent 50min on the problem and 10min on the solution, not vice versa!"

Don't get me wrong: I do acknowledge the issue and can reproduce it fairly easy on the sap.ui.comp.navpopover.SmartLink control - but I myself didn't figure its actual root cause yet. I do not know the problem well enough to start talking about a solution.

The actual root cause is a circular reference

If you found the root cause - awesome!

So I assume you are refering to a specific circular reference then (as in general, circular references shouldn't be an issue). So here are my thoughts:

• Why is this particular circular reference an issue while others aren't?
• And with respect to the used postMessage method, its provided API and documented restrictions: Shouldn't it be able to handle this circular reference?
  • If so, it is a bug/issue in Chrome/V8 and should be reported.
  • If not, then how to detect/prevent such a specific circular reference? (here finally comes the actual fix into play)

[SeeSharpSoft]

@gtreskas Seems that I finally reached the state you were already in for a bit (my apologies), realizing circular references in general are indeed a problem for port.postMessage method, which is a different one than mentioned here.

I also noticed this on additional controls, e.g. sap.ushell.components.container.ApplicationContainer. There are references to the sap.ui.Core instance within its control data 😮

Possible solutions

• Ignore generally already processed references (what @gtreskas mentioned in the first post): This does not only prevent circular references but also non-circular references to already processed parts. How relevant are those references?
• Detecting and preventing only circular dependencies: Probably decreases the performance (notably?). Is it sufficient?
• Ignoring controls/messages with a circular dependeny (basically catching JSON.stringify): User might miss information.

In all three cases, data gets potentially lost, which seems inevitable. Or do I miss something? Is there another solution? What is most favorable?

[gtreskas]

@SeeSharpSoft :Yes thank you very much for taking your time and having a look, and stating the facts. Yes you are right is inevitable to keep everything, we have to loose something here, but in this case we should see if what we loose is ok or not. I think the best would be to understand what is the data that we are loosing and if its important for the inspector...

Here we have a tradeoff which we have also to consider , which you rightfully state: Data Amount vs. Performance, as more data we include in the inspector as slower and dangerous (circular refs) it becomes..

The thing is here for me, circular references is no go, this has to be handled and fixed. About performance maybe we need to adjust the existing architecture of the inspector and require "smaller" portions of data packed into "smaller messages". I think this could correct the performance significantly, since we will only require "specific" portions of data and not everything at once...

But maybe the other colleagues may have a better ideas here, what do you say colleagues?

[SeeSharpSoft]

@gtreskas Just pushed a rework of the message parsing algorithm, removing circular references (2) but also ignoring complex object types.

I am with you: The performance in those cases is super bad even when ignoring all references, but when applying the check for circular references it is literally unusable.

Manual sanity checks with the references change are looking good. I also checked what happens when ignoring all references (1): Then e.g. the context binding information in the bindings tab gets already lost, so this is actually not really an option.

[gtreskas]

@SeeSharpSoft Awesome! Thank you very much for your work, i didnt had time lately, but I will try it out and let you know! I will pull the changes from your PR.

[jdichev]

Hello,

This should have been resolved in latest versions.

Best regards,
Jordan