Remove reflections: Misc and Summary #1137

karakasa · 2023-07-28T15:33:22Z

try to fix #957

I found some upstream dependencies don't support AOT therefore NPOI cannot be completed AOTed. The library will get some performance improvement from removing reflections though.

@tonyqus What are your thoughts on this? These are the issues I've witnessed.

Following features relies on reflection.

Feature	Description	Mitigation
`XWPFTableRow.CloneRow`	Relies on deepcopying `CT_*` objects, hence `NPOI.Util.ObjectExtensions.Copy<T>()`.	Needs testing. Probably keep metadata by `rd.xml` or source generators for deepcopy. Perhaps it will just work.
XML (de)serialization, as well as `NPOI.OpenXml4Net.Util.XmlHelper`	`XMLSerializer` and enum names relies on reflections obviously.	It looks like AOT compiler handles well. Thorough tests are needed.

Following methods are converted from dynamic dispatch to static lookup. These may prevent some later-injected custom encryptor, record types, etc. I think it's fine. Maybe it's better to use source generators. They will lead to a slight increase of binary size.

Class	Method
`NPOI.DDF.DefaultEscherRecordFactory`	Entire Class	#1159
`NPOI.HSSF.Record.RecordFactory`	Entire Class	#1159
`NPOI.POIFS.Crypt.EncryptionInfo`	`GetBuilder`	#1160
`NPOI.XSSF.UserModel.XSSFFactory`	`CreateDocumentPart`	#1157
`NPOI.XSSF.UserModel.XWPFFactory`	`CreateDocumentPart`	#1157

The following classes / methods, using reflection, are useless.

Target	Reason
`NPOI.HSSF.UserModel.OperationEvaluatorFactory`	Wrong use of reflection. Maybe incorrectly ported from Java.	#1161
`NPOI.POIFS.NIO.FileBackedDataSource.unmap`	Entire method contains no code. Seems Java-specific.	#1161
`NPOI.SS.Formula.Eval.Forked.ForkedEvaluator.CreateEvaluationWorkbook`	Don't work at all because of typo. 'NPOI.XSSF.UserMode', forgetting the last 'l'	#1161
`NPOI.OpenXml4Net.Util.ZipSecureFile.AddThreshold`	The code is used to deal with ZIP bombs but Java-specific. We should come up with ways in CLR.	#1161
`NPOI.Util.POILogFactory.GetLogger`	I doubt if the following line would work, because `_loggerClassName` is `Name` of the type but `Type.GetType` requires `FullName`. It all ends up using the null logger.	#1162

Others

Feature	Description
`NPOI.SS.Formula.OperationEvaluatorFactory.Add`	Reflection is used to ensure singleton-pattern. Can be removed if we're sure.	this
`Ptg`, `OperandPtg`	`IClonable` is removed from `Ptg` because not used and requires deepcopy, except for `OperandPtg`. After inspection, all derived classes of `OperandPtg` contain only value-type members so `MemberwiseClone` is enough. (besides, `Ptg.IClonable.Clone()` throws `NotImplementedException` so I doubt anyone is using it.)	this
`HSSFColor`	Slightly refactored	#1158
`(T[])ArrayList.ToArray(typeof(T))`	Rewritten as extension method	this

Dependencies AOT compiler complains about. Not necessarily non-AOTable. It's possible to remove some of them.

Library	State	Comments
System.Configuration.ConfigurationManager		Produce trim warnings. Solely used in `NPOI.Util.SystemOutLogger` & `NPOI.Util.POILogFactory`	Not dealt with
Enums.NET	Removed	Relies on Reflection. Converted into static dispatch & built-in methods	#1156

Probably need to add net8.0 as another target since .NET 6 doesn't fully support AOT and AOT analyzers. But the Github CI/action don't support .NET 8.
Generally there's no breaking change, except Remove reflections in POI Logger #1162

…into remove_reflection

karakasa · 2023-07-29T14:24:42Z

btw comparsion of loading XSSF file. The performance is better for smaller files. And it looks like no obvious performance gains from removing Reflections alone.

File	Version	Time (s)
Large (30M)	PR w/ AOT	35.687
Large (30M)	PR	36.676
Large (30M)	Nuget	36.086
Small (600K)	PR w/ AOT	0.332
Small (600K)	PR	0.747
Small (600K)	Nuget	0.765

Nuget: the nuget version.

karakasa · 2023-08-12T06:02:38Z

OpenXmlFormats/OOXMLFactory.cs

@@ -22,7 +22,7 @@ public T Parse(Stream stream)
        }
        public T Create()
        {
-            return (T)Activator.CreateInstance(typeof(T));
+            return new T();


Although new T() uses Activator.CreateInstance internally, this semantic is slightly different for AOT analyzers I believe.

karakasa · 2023-08-12T08:44:25Z

main/SS/Formula/PTG/OperandPtg.cs

@@ -37,7 +37,8 @@ public OperandPtg Copy()
        {
            try
            {
-                return (OperandPtg)Clone();
+                // REMOVE-REFLECTION: After careful inspection, MemberwiseClone() should be enough for all built-in OpreandPtgs.
+                return (OperandPtg)MemberwiseClone();


All derived classes of OperandPtg holds only value types (some unused members are not, but that's fine).

karakasa · 2023-08-12T08:44:52Z

main/SS/Formula/PTG/Ptg.cs

+        // private virtual object Clone()
+        //{
+            //return this.Copy();
+        //}


This is not used elsewhere. And IClonable.Clone throws NotImplementedException, see below.

tonyqus · 2023-08-13T14:19:33Z

I created a new branch called AOT. AOT branch will become a RC version for NPOI vNext. I'd like to compare the performance between AOT and non-AOT release.

josephmoresena · 2024-08-01T22:57:45Z

I created a new branch called AOT. AOT branch will become a RC version for NPOI vNext. I'd like to compare the performance between AOT and non-AOT release.

Thanks, I'm here if you want some help.

karakasa and others added 5 commits July 28, 2023 23:29

removed many depedencies on System.Reflection

450904f

Merge branch 'nissl-lab:master' into remove_reflection

4553a9c

removed more reflection depdencies

d2e2d01

Merge branch 'remove_reflection' of https://github.com/karakasa/npoi …

830c57b

…into remove_reflection

fixed typo

cf3cbed

karakasa changed the title ~~Remove reflection dependencies~~ Remove reflection dependencies and prepare NPOI for AOT Jul 28, 2023

karakasa added 4 commits July 29, 2023 03:08

fixed typo

88955fd

fixed class reference

c6438a5

fix more AOT issues

0763801

fixed solution file

192d221

This comment was marked as outdated.

Sign in to view

added net8 target; removed Enums.NET

b934822

Merge branch 'nissl-lab:master' into remove_reflection

73d4ccf

This comment was marked as outdated.

Sign in to view

karakasa added 13 commits August 12, 2023 12:49

make Enum parsers throw the correct Exception

734b5a4

Split enum-related to anthoer branch

35aeb92

Fix names

07509fb

split files

6123227

split files

c8f45f9

Split files outside

0d36498

clean files

d482dc0

clean files

5cdebc2

Clean files

3e7e32e

Clean files

a462ee9

Clean files

5a6c4ec

clean files

9c6ddb9

clean files

9ab9280

karakasa commented Aug 12, 2023

View reviewed changes

karakasa changed the title ~~Remove reflection dependencies and prepare NPOI for AOT~~ Remove reflections: Misc and Summary Aug 12, 2023

karakasa marked this pull request as ready for review August 12, 2023 06:13

karakasa commented Aug 12, 2023

View reviewed changes

fix bugs

01652ab

tonyqus changed the base branch from master to aot August 13, 2023 14:18

tonyqus added this to the NPOI 2.7.1 milestone Aug 13, 2023

tonyqus added the aot label Aug 17, 2023

tonyqus merged commit 96836a6 into nissl-lab:aot Oct 11, 2023
2 checks passed

karakasa deleted the remove_reflection branch October 12, 2023 03:34

tonyqus modified the milestones: NPOI 2.7.1, NPOI AOT May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove reflections: Misc and Summary #1137

Remove reflections: Misc and Summary #1137

karakasa commented Jul 28, 2023 •

edited

Loading

This comment was marked as outdated.

karakasa commented Jul 29, 2023 •

edited

Loading

This comment was marked as outdated.

karakasa Aug 12, 2023

karakasa Aug 12, 2023

karakasa Aug 12, 2023

tonyqus commented Aug 13, 2023

josephmoresena commented Aug 1, 2024

Remove reflections: Misc and Summary #1137

Remove reflections: Misc and Summary #1137

Conversation

karakasa commented Jul 28, 2023 • edited Loading

This comment was marked as outdated.

karakasa commented Jul 29, 2023 • edited Loading

This comment was marked as outdated.

karakasa Aug 12, 2023

Choose a reason for hiding this comment

karakasa Aug 12, 2023

Choose a reason for hiding this comment

karakasa Aug 12, 2023

Choose a reason for hiding this comment

tonyqus commented Aug 13, 2023

josephmoresena commented Aug 1, 2024

karakasa commented Jul 28, 2023 •

edited

Loading

karakasa commented Jul 29, 2023 •

edited

Loading