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

Reduce code duplication due to reified generics #1444

Merged
merged 1 commit into from
Nov 27, 2020

Conversation

Sergio0694
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

The BokehBlurProcessor<TPixel> type had one of the internal row iterators that worked on non-generic types, but having that nested type defined within the generic processor class caused the code to be duplicated and compiled again for every single pixel type that the process was applied to. I moved that to the non-generic processor type so the same generic instantiation can be reduced. This will reduce the binary size in AOT scenarios, and reduce the load on the JIT otherwise.

@@ -127,7 +127,7 @@ protected override void OnFrameApply(ImageFrame<TPixel> source)
in verticalOperation);

// Compute the horizontal 1D convolutions and accumulate the partial results on the target buffer
var horizontalOperation = new ApplyHorizontalConvolutionRowOperation(sourceRectangle, processingBuffer, firstPassBuffer, kernel, parameters.Z, parameters.W);
var horizontalOperation = new BokehBlurProcessor.ApplyHorizontalConvolutionRowOperation(sourceRectangle, processingBuffer, firstPassBuffer, kernel, parameters.Z, parameters.W);
Copy link
Member

Choose a reason for hiding this comment

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

Can you not reference the processor passed to the constructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean - this is a static reference, we're not accessing an instance member. That's just the qualified type that's declared within BokehBlurProcess, so we need the parent type name when using it 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I misread, sorry, carry on. 👍

@JimBobSquarePants
Copy link
Member

@Sergio0694 Nice! Always on the lookout for things like this. We need to ensure the amount of code we produce is as small as possible to avoid issues with AOT.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@codecov
Copy link

codecov bot commented Nov 27, 2020

Codecov Report

Merging #1444 (05659b8) into master (718945c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1444   +/-   ##
=======================================
  Coverage   83.68%   83.68%           
=======================================
  Files         734      734           
  Lines       31990    31990           
  Branches     3605     3605           
=======================================
  Hits        26772    26772           
  Misses       4505     4505           
  Partials      713      713           
Flag Coverage Δ
unittests 83.68% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ssing/Processors/Convolution/BokehBlurProcessor.cs 100.00% <100.00%> (ø)
...ocessors/Convolution/BokehBlurProcessor{TPixel}.cs 98.85% <100.00%> (-0.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 718945c...05659b8. Read the comment docs.

@JimBobSquarePants JimBobSquarePants merged commit cad52a2 into master Nov 27, 2020
@JimBobSquarePants JimBobSquarePants deleted the sp/bokeh-kernel-jit-opt branch November 27, 2020 16:39
JimBobSquarePants added a commit that referenced this pull request Mar 13, 2021
Reduce code duplication due to reified generics
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.

2 participants