pnp / pnp/pnpframework

Question regarding "TokenParser" because of pending optimizations

Open
#963 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C#
Stars
259
Forks
161
Avg merge
10h 33m
Merged PRs (30d)
1

Description

Hello PnP team,

I have some question regarding parts of the TokenParser.

Those question arose when I analyzed and optimized the performance of the TokenParser, because PnP.Framework had a really bad one when I tried to deploy a fairly large template. It took unusually long to deploy a field, a content type or a list (up to minutes). Also the initialization of the TokenParser alone took over 10 minutes on an S2 App Service Plan and 1.5 minutes on an i7-12800HX. Needless to say, that the CPU usage was really high.

With the usage of a profiler I was able to pinpoint the initialization issue to how the localization resource tokens were added to the parser. The template in questions defined a larger amount of languages and therefore also a larger amount of resource tokens. At the end the parser had about 48,000 definitions and above 300,000 tokens.

A second issue was the (re)building of the cache, which happened every time a string should be parsed. I know that a token gets skipped if he is already a part of the cache, but iterating through all definitions and their tokens alone in combination with using Regex.Unescape every time to then just skip it, is really taxing the CPU. This was also something the profile has shown.

I already made changes to the TokenParser and was able to reduce the initialization time on my machine from about 1.5 minutes to 35 seconds. The processing of the resource tokens alone required 1 minute and has been reduced to about a second. By the way the culprit was the LINQ operation looking for matching entries at the end.

I will create a pull request and link this issue to it. I might also create additional commits based on the outcome of this discussion.

Now my questions.

Rebase(Web) only calls ClearCache on every definition, but does not clear the actual cache of the TokenParser. Therefore clearing the cache of a TokenDefinition would only affect non-cacheable definitions. Is this an intended behavior?


AddToken and the constructor accepting a web, a template and apply information always sort the token definitions based their longest token in a descending order. Is this still required? If I haven't overlooked something, the order is not important for any of the operations. Is my assumption correct and the sorting could be removed?

This is a source for a high CPU utilization. Especially when performed every time a token gets added by using AddToken.

If the order is still required, adding a method like AddTokens would be a solution. It would allow adding multiple tokens at once. The code of the ObjectHandler adding tokens must be changed to add all at once (if possible).


RemoveToken does not update the ListTokenDictionary dictionary. Is this intended or an oversight?


It looks like Clone, or more specific the private constructor used by it does not really clone the TokenParser and just returns a new instance using the same reference to the _tokens list. ObjectListInstance uses the method to not pollute the global parser while working with a list.

Because the cloned TokenParser uses the same reference to the TokenDefinition list, manipulating the parser has a direct impact on the global parser and therefore defeating the whole purpose of it.

My suggestion would be to remove Clone from TokenParser and modifying ObjectListInstance to not use it anymore. At the moment every list provisioning operation "pollutes" the global TokenParser and it looks like there are no issues regarding this. Removing the cloning would also eliminate any other operation the cloned TokenParser would perform when working with the tokens (because those information are not "cloned"), which additionally reduces the required CPU time.

If Clone must remain, an idea would be something like a "sub parser", which uses the already prepared information provided by the source and would only maintain token being added after the cloning happened. Actually copying the whole list of token definitions etc. every time is in my opinion too much.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with src/lib/PnP.Framework/Provisioning/ObjectHandlers/TokenParser.cs and the linked methods AddToken, Rebase, RemoveToken, and Clone, then inspect ObjectListInstance.cs where the clone is used. Reproduce or profile the parser behavior described in the issue and determine the intended cache, ordering, dictionary, and cloning semantics; done means the questions are resolved and any agreed optimization is covered by appropriate tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend, performance
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.