Skip to content

WIP: Use call_once for dynamic loading ObjectFactoryBase::Initialize()#4227

Closed
N-Dekker wants to merge 3 commits intoInsightSoftwareConsortium:masterfrom
N-Dekker:ObjectFactoryBase-call_once
Closed

WIP: Use call_once for dynamic loading ObjectFactoryBase::Initialize()#4227
N-Dekker wants to merge 3 commits intoInsightSoftwareConsortium:masterfrom
N-Dekker:ObjectFactoryBase-call_once

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker commented Sep 21, 2023

Removed ObjectFactoryBasePrivate::m_Initialized. Moved RegisterInternal()
call from ObjectFactoryBase::Initialize() to the default-constructor of
ObjectFactoryBasePrivate.


Also cherry-picked the commit from pull request #4225, STYLE: Remove private ObjectFactoryBase member InitializeFactoryList(). (This PR is not meant to supersede PR #4225)

`ObjectFactoryBase::InitializeFactoryList()` has become unnecessary from
pull request InsightSoftwareConsortium#3641
commit 48d0cb0 "STYLE: Remove pointer
indirection from lists of ObjectFactoryBasePrivate"
@github-actions github-actions Bot added the area:Core Issues affecting the Core module label Sep 21, 2023
`RegisterInternal()` is a private member function of ObjectFactoryBase. It is
only called during the initialization of m_PimplGlobals.
@N-Dekker N-Dekker force-pushed the ObjectFactoryBase-call_once branch from af2067e to 8e44857 Compare September 22, 2023 08:13
@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Sep 22, 2023

Please note: this is really still in an experimental state. I still need to figure out exactly how things work! (The other PR, #4225 is ready though, independent of the discussion on atomic vs mutex vs call_once)

Removed `ObjectFactoryBasePrivate::m_Initialized`. Removed `RegisterInternal()`
call from `ObjectFactoryBase::Initialize()`.
@N-Dekker N-Dekker force-pushed the ObjectFactoryBase-call_once branch from 8e44857 to 2610540 Compare September 22, 2023 17:47
@issakomi
Copy link
Copy Markdown
Member

issakomi commented Sep 24, 2023

A little off topic. Interesting what OpenGL factory is mentioned here :)

/**
 * Register any factories that are always present in ITK like
 * the OpenGL factory, currently this is not done.
 */
void
ObjectFactoryBase::RegisterInternal()

It looks like the class was a long time ago copied from VTK, s. comment in vtkObjectFactory.cxx.

@N-Dekker
Copy link
Copy Markdown
Contributor Author

A little off topic. Interesting what OpenGL factory is mentioned here :)

/**
 * Register any factories that are always present in ITK like
 * the OpenGL factory, currently this is not done.
 */
void
ObjectFactoryBase::RegisterInternal()

Thanks for your input, @issakomi I'm now proposing to just remove RegisterInternal(), including the apparently VTK specific comment about "the OpenGL factory". Please check:

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Oct 5, 2023

I'm afraid I was mistaken when I was thinking that call_once might be useful for ObjectFactoryBase::Initialize(). The member function claims to be a "one time initialization method":

/**
* A one time initialization method.
*/
void
ObjectFactoryBase::Initialize()

But in fact, it may be called more than once, on the very same ObjectFactoryBase instance, by calling ReHash:

void
ObjectFactoryBase::ReHash()
{
ObjectFactoryBase::UnRegisterAllFactories();
ObjectFactoryBase::Initialize();
}

So I guess we may need to reconsider using a mutex (PR #4226?)... at least if we think we have serious thread-safety problem here.

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Feb 3, 2024

Abandoned: this approach (using call_once) does not work, because of ReHash as I mentioned before at #4227 (comment)

@N-Dekker N-Dekker closed this Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants