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

Merge make functions into getters. #1482

Open
wants to merge 2 commits into
base: staging/AOT
Choose a base branch
from

Conversation

jlaanstra
Copy link
Collaborator

Results in small size gain.

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Have you measured any size change with NAOT?
Can you include a screenshot with before/after with sizoscope?

src/cswinrt/code_writers.h Outdated Show resolved Hide resolved
@jlaanstra
Copy link
Collaborator Author

Have you measured any size change with NAOT? Can you include a screenshot with before/after with sizoscope?

I haven't tested with any NativeAOT scenario.

@Sergio0694
Copy link
Member

Mmh actually looking at updated projections I see this:

private static IObjectReference _objRef_global__Microsoft_Graphics_Canvas_CanvasDevice
{
  get
  {
    IObjectReference canvasCanvasDevice = CanvasDevice.___objRef_global__Microsoft_Graphics_Canvas_CanvasDevice;
    return canvasCanvasDevice != null && canvasCanvasDevice.IsInCurrentContext ? canvasCanvasDevice : (CanvasDevice.___objRef_global__Microsoft_Graphics_Canvas_CanvasDevice = ActivationFactory.Get("Microsoft.Graphics.Canvas.CanvasDevice"));
  }
}

public CanvasDevice()
  : this((IObjectReference) IActivationFactoryMethods.ActivateInstance<IUnknownVftbl>(CanvasDevice._objRef_global__Microsoft_Graphics_Canvas_CanvasDevice))
{
  ComWrappersSupport.RegisterObjectForInterface((object) this, this.ThisPtr);
  ComWrappersHelper.Init(this._inner, false);
}

Can you explain that canvasCanvasDevice != null && canvasCanvasDevice.IsInCurrentContext ? canvasCanvasDevice : (CanvasDevice.___objRef_global__Microsoft_Graphics_Canvas_CanvasDevice = ActivationFactory.Get("Microsoft.Graphics.Canvas.CanvasDevice")); bit? Isn't that just going to always throw the activation factory away every single time and just overwrite it with a new one if someone frequently creates new instances from different threads? Am I missing something here about how that works?

@jlaanstra
Copy link
Collaborator Author

Mmh actually looking at updated projections I see this:

private static IObjectReference _objRef_global__Microsoft_Graphics_Canvas_CanvasDevice
{
  get
  {
    IObjectReference canvasCanvasDevice = CanvasDevice.___objRef_global__Microsoft_Graphics_Canvas_CanvasDevice;
    return canvasCanvasDevice != null && canvasCanvasDevice.IsInCurrentContext ? canvasCanvasDevice : (CanvasDevice.___objRef_global__Microsoft_Graphics_Canvas_CanvasDevice = ActivationFactory.Get("Microsoft.Graphics.Canvas.CanvasDevice"));
  }
}

public CanvasDevice()
  : this((IObjectReference) IActivationFactoryMethods.ActivateInstance<IUnknownVftbl>(CanvasDevice._objRef_global__Microsoft_Graphics_Canvas_CanvasDevice))
{
  ComWrappersSupport.RegisterObjectForInterface((object) this, this.ThisPtr);
  ComWrappersHelper.Init(this._inner, false);
}

Can you explain that canvasCanvasDevice != null && canvasCanvasDevice.IsInCurrentContext ? canvasCanvasDevice : (CanvasDevice.___objRef_global__Microsoft_Graphics_Canvas_CanvasDevice = ActivationFactory.Get("Microsoft.Graphics.Canvas.CanvasDevice")); bit? Isn't that just going to always throw the activation factory away every single time and just overwrite it with a new one if someone frequently creates new instances from different threads? Am I missing something here about how that works?

That is exactly what it does. We only cache one factory and if the cached version requires a different apartment, we create a new one and cache that. For free-threaded factories we cache the first one forever (IsInCurrentContext will always be true).

src/cswinrt/code_writers.h Outdated Show resolved Hide resolved
src/cswinrt/code_writers.h Outdated Show resolved Hide resolved
@jlaanstra jlaanstra force-pushed the user/jlaans/remove-make-functions branch from 210313b to c895b0d Compare March 13, 2024 21:37
@jlaanstra jlaanstra force-pushed the user/jlaans/remove-make-functions branch from c895b0d to ba34694 Compare March 13, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants