Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: Reduce nodetype schema size #4561

Merged
merged 4 commits into from Oct 25, 2023
Merged

Conversation

Sebobo
Copy link
Member

@Sebobo Sebobo commented Sep 26, 2023

With this change the following optimisations are done to improve speed and reduce size of the schema generation:

  • Abstract nodetypes are not queried anymore for constraints as they are already resolved by the nodetype manager.

  • Entries in the inheritance map and constraints will be skipped if they don’t contain any data.

These optimisations reduce the size of the schema in the Neos.Demo from ~357KB to ~300KB and improve the response time by ~20% in my tests.

The more nodetypes a project has, the bigger the benefit is.

Review instructions

Everything should work the same, adding nodes, constraints, etc.

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

With this change the following optimisations are done to improve speed and reduce size of the schema generation:

* Abstract nodetypes are not queried anymore for constraints as they
are already resolved by the nodetype manager.

* Entries in the inheritance map and constraints will be skipped if they don’t contain any data.

These optimisations reduce the size of the schema in the Neos.Demo from ~380KB to ~307KB and improve the response time by ~20%.

The more nodetypes a project has, the bigger the benefit is.
@Sebobo
Copy link
Member Author

Sebobo commented Sep 26, 2023

I wanted to also skip the abstract types in the first loop, but the Neos.UI seems to need at least Neos.Neos:Document and Neos.Neos:Content in the inheritanceMap array.

Edit: I guess the entries in Neos.Neos.Ui.nodeTypeRoles are required.

@Sebobo Sebobo marked this pull request as ready for review October 13, 2023 12:11
@Sebobo Sebobo requested a review from mhsdesign October 13, 2023 12:12
@Sebobo
Copy link
Member Author

Sebobo commented Oct 13, 2023

Example from a customer project:

  • their schema was 23MB and with this change is 20MB.
  • response time was 4.1s and now is 3.2s.

Now their biggest chunk is broken nodetypes which are neither abstract nor have super types. Those are massively enriched by the schema builder with useless information.

Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all makes sense to me, thanks! I kinda see the need to rethink how this schema works, or rather how we build it, seems super convoluted, but that's another task.

Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, I left just one small question inline…

Neos.Neos/Classes/Service/NodeTypeSchemaBuilder.php Outdated Show resolved Hide resolved
@Sebobo
Copy link
Member Author

Sebobo commented Oct 23, 2023

Broke some tests, will check.

@Sebobo
Copy link
Member Author

Sebobo commented Oct 25, 2023

@mhsdesign Tests run again, are you also ok with the change?

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes ;)

@Sebobo Sebobo merged commit c7306eb into 8.3 Oct 25, 2023
7 checks passed
@Sebobo Sebobo deleted the bugfix/reduce-nodetype-schema-size branch October 25, 2023 14:47
mhsdesign added a commit that referenced this pull request Oct 31, 2023
@mhsdesign
Copy link
Member

As we decided to move this either way to the neos ui, i eased the upmerge by ignoring your change for now.

See issue neos/neos-ui#3658

@lorenzulrich
Copy link
Contributor

@Sebobo @mhsdesign It seems this breaks a feature of https://github.com/sitegeist/Sitegeist.Archaeopteryx and I'm not sure how exactly. In this package, I can configure the baseNodeType and allowedNodeTypes of a link.

In Neos 8.3.6, I could use Abstract Node Types in the configuration:

'Sitegeist.Archaeopteryx:Node':
  baseNodeType: 'FoobarCom.Site.Base:Document.Abstract.Page'
  allowedNodeTypes: ['FoobarCom.Site.Base:Document.Abstract.LinkablePage']

From 8.3.7, this does not work anymore.

In 8.3.6, the Node Type Schema returned abstract types including their children:

"FoobarCom.Site.Base:Document.Abstract.LinkablePage": [
	"FoobarCom.Site.Base:Document.KnowledgeBaseEntryPage",
	"FoobarCom.Site.Base:Document.RootPage",
	"FoobarCom.Site.Base:Document.Page",
	"FoobarCom.Site.Base:Document.KnowledgeBasePage",
	"FoobarCom.Site.Region:Document.RootPage",
	"FoobarCom.Site.Corporate:Document.RootPage"
],

In 8.3.7, this block is completely missing. Is there another way Sitegeist.Archaeopterix could resolve the node types beloging to such an Abstract type?

@Sebobo
Copy link
Member Author

Sebobo commented Dec 29, 2023

@Sebobo @mhsdesign It seems this breaks a feature of https://github.com/sitegeist/Sitegeist.Archaeopteryx and I'm not sure how exactly. In this package, I can configure the baseNodeType and allowedNodeTypes of a link.

In Neos 8.3.6, I could use Abstract Node Types in the configuration:


'Sitegeist.Archaeopteryx:Node':

  baseNodeType: 'FoobarCom.Site.Base:Document.Abstract.Page'

  allowedNodeTypes: ['FoobarCom.Site.Base:Document.Abstract.LinkablePage']

From 8.3.7, this does not work anymore.

In 8.3.6, the Node Type Schema returned abstract types including their children:


"FoobarCom.Site.Base:Document.Abstract.LinkablePage": [

	"FoobarCom.Site.Base:Document.KnowledgeBaseEntryPage",

	"FoobarCom.Site.Base:Document.RootPage",

	"FoobarCom.Site.Base:Document.Page",

	"FoobarCom.Site.Base:Document.KnowledgeBasePage",

	"FoobarCom.Site.Region:Document.RootPage",

	"FoobarCom.Site.Corporate:Document.RootPage"

],

In 8.3.7, this block is completely missing. Is there another way Sitegeist.Archaeopterix could resolve the node types beloging to such an Abstract type?

Yes there is a workaround in neos/neos-ui#3677 documented.
We weren't aware of any use of the API and I'm surprised that the plugin needs that as the backend should resolve the nodetypes 🧐

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants