Cache context offline device setting specified by environment variables

[sophimao]

Currently runtime queries context offline device setting from environment variables whenever this setting is needed, therefore introduces performance overhead. The caching is done to avoid this performance overhead.

[sophimao]

This is the first stage change to refactor the context offline mode setting, full change will come later in #234.
Verified that this resolves the regression introduced in #214

[zibaiwan]

Thanks @sophimao ! Looks good to me!

[pcolberg]

Thanks @sophimao, looks good. Please see two minor nitpicks below.

There remain two further calls of acl_get_offline_device_user_setting() as detailed below. Why are these still needed, i.e., why can't they use the cached setting? This might be worthwhile commenting on in the commit message.

fpga-runtime-for-opencl/src/acl_globals.cpp

Lines 161 to 173 in 3c5c4f2

	// Initialize the HAL and load the builtin system definition.
	//
	// It will handle setting up an offline device, either exclusively (by
	// default), or in addition to devices found via HAL probing.
	//
	// This function returns CL_TRUE if a hal is initialized and CL_FALSE
	// if it is not.
	cl_bool acl_init_from_hal_discovery(void) {
	int use_offline_only = 0;
	const acl_hal_t *board_hal;
	acl_assert_locked();
	(void)acl_get_offline_device_user_setting(&use_offline_only);

fpga-runtime-for-opencl/src/acl_hal_mmd.cpp

Lines 1250 to 1251 in 3c5c4f2

	// Dynamically load board mmd & symbols
	(void)acl_get_offline_device_user_setting(&use_offline_only);

[sophimao]

There remain two further calls of acl_get_offline_device_user_setting() as detailed below. Why are these still needed, i.e., why can't they use the cached setting?

These are called before the variable is cached. The reason why I cached the variable in acl_init_platform is because it is the function that both acl_init and acl_init_from_hal_discovery calls. Another option is to add a acl_get_offline_device_user_setting call to acl_init and do the caching in acl_init/acl_init_from_hal_discovery.

I personally think it is cleaner to cache the variable in the acl_init_platform function, as the cache is a member of acl_platform, and the two remaining calls shouldn't add too much overhead, but I'm open to the other option if you have a reason for going with it. If you don't strongly prefer the other option then I can add this explanation to the commit message :)

[pcolberg]

There remain two further calls of acl_get_offline_device_user_setting() as detailed below. Why are these still needed, i.e., why can't they use the cached setting?

These are called before the variable is cached. The reason why I cached the variable in acl_init_platform is because it is the function that both acl_init and acl_init_from_hal_discovery calls. Another option is to add a acl_get_offline_device_user_setting call to acl_init and do the caching in acl_init/acl_init_from_hal_discovery.

I see. The call graph here is

acl_init_from_hal_discovery()
├── acl_mmd_get_system_definition()
└── acl_init_platform()

so to avoid multiple calls, acl_get_offline_device_user_setting() would need to be called in acl_init_from_hal_discovery(), correct? The setting could then be passed to acl_mmd_get_system_definition() and acl_init_platform() as an argument, for instance.

I personally think it is cleaner to cache the variable in the acl_init_platform function, as the cache is a member of acl_platform, and the two remaining calls shouldn't add too much overhead, but I'm open to the other option if you have a reason for going with it. If you don't strongly prefer the other option then I can add this explanation to the commit message :)

Yes, this is about design only, and now I think your commit is fine without further comment. Let's keep this commit as is for 2023.1 and work out the full design in #234.

[sophimao]

so to avoid multiple calls, acl_get_offline_device_user_setting() would need to be called in acl_init_from_hal_discovery(), correct?

Yes, for the regular flow that is correct. However, for unit test, there is another call graph:

acl_init()
└── acl_init_platform()

So we would also need to add a call to acl_get_offline_device_user_setting in acl_init to pass the value around.

I will keep the design this way for now, and address this in #234.

[pcolberg]

so to avoid multiple calls, acl_get_offline_device_user_setting() would need to be called in acl_init_from_hal_discovery(), correct?

Yes, for the regular flow that is correct. However, for unit test, there is another call graph:

acl_init()
└── acl_init_platform()

Of course, I missed that despite being right above the other entry point 🤦

So we would also need to add a call to acl_get_offline_device_user_setting in acl_init to pass the value around.

I will keep the design this way for now, and address this in #234.

The separate entry points for testing and production are unfortunate, but that topic should probably be handled separately from the revision in #234. I suggest you rebase that PR onto the main branch and narrow the focus on refactoring acl_get_offline_device_user_setting() to remove the free-form string return value (if that makes sense).