-
Notifications
You must be signed in to change notification settings - Fork 635
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
shrink the size of the service core runtime nuget #15295
Conversation
dont attempt to write with lucene dont copy docs for openxml dont copy samples,licenses,or node docs
test/Libraries/DynamoUtilitiesTests/DynamoUtilitiesTests.csproj
Outdated
Show resolved
Hide resolved
UI Smoke TestsTest: success. 3 passed, 0 failed. |
@@ -31,7 +31,7 @@ | |||
<Isolated>False</Isolated> | |||
<EmbedInteropTypes>True</EmbedInteropTypes> | |||
</COMReference> | |||
<PackageReference Include="DocumentFormat.OpenXml" Version="2.12.3" CopyXML="true" /> | |||
<PackageReference Include="DocumentFormat.OpenXml" Version="2.12.3" CopyXML="false" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this change affect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think it affects dynamo users at all- but it keeps a 12 megabyte xml file out of the bin folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what XML file is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not needed for Dynamo.All anymore ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, but I will double check.
<file src="**" target="lib\$TargetFramework$\" exclude="en-US\fallback_docs\*"/> | ||
<file src="**" | ||
target="lib\$TargetFramework$\" | ||
exclude="en-US\fallback_docs\*;NodeHelpSharedDocs\*;Open Source Licenses\*;samples\** "/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aparajit-pratap @pinzart90 do you think it makes sense to invert this and manually include every file we want to deploy so that changes to Dynamo bin folder don't automatically make it to the service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, so this right now will deploy all of the dll's in the bin folder into the service, even those that are not part of the service runtime core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have been more clear.
Right now, the way the job is setup for this nuget and build - it will deploy just the DynamoCore.Sln
- I was just thinking of someone in the future added something to that .sln it will then make into the nuget package automatically. (it's a separate build job from our normal ones)
My worry is that it's not clear what else this DynamoCore.sln might be used for in the future besides the service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying. I think this is good enough for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is easier to control what dlls and content in general within the csproj (more tools at our disposal)
I think we should have a solution (or configs in general) that produce on disk exactly what the service is. It will be valuable to build exactly what will be inside the docker container.
Maybe we should rename the sln to DynamoService.sln (if that is makes it clearer to anyone)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nugget config might help with filtering out test assemblies.
Or maybe that can be done earlier...(dynamo.All needs that too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a solution (or configs in general) that produce on disk exactly what the service is. It will be valuable to build exactly what will be inside the docker container.
I agree. I will think about if we should change the name, add a config, or add a new .sln.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment
I am fine with the changes in this PR for now if you want to get another PR for other stuff |
unfortunately there appear to be a few test failures I need to look at first. |
Merging for now. We can follow up on the remaining issues |
Purpose
This my first attempt to shrink the size of the service core runtime nuget. I plan on sending a few more PRs, each removing functionality that might be less obvious / more contentious or require larger changes.
This is done by:
introducing a constantinstead split the assemblies and use interfaces or some other decoupling method. For example, it would be nice to move all the lucene code to another assembly and reference it with an interface in#MINIMAL_SERVICE
to control compiling some code that references the removed projects.I think long term it would be good to remove this constant andDynamoServices
, likeISearchBackend
- so we can totally avoid needing to ship all the lucene dependencies where we are never going to use them.IsServiceMode
to control a few places that took a bit of time in profiling - compared to ASM loading they are mostly inconsequential though they do avoid some file system access.isHeadless
andCLIMode
since lucene indexing was still happening in the CLIs. (still TODO)FYI - it would be great if we could find some time to align all these wacky overlapping config settings...
Any others?
In the end the nupkg goes 275 megs to 104 megs.
Starting the CLI in service mode is now 400 ms faster (measured with diagnostics.stopwatch).