Cache context offline device setting specified by environment variables (2nd stage)

[sophimao]

The context offline mode is set through environment variables and remains constant throughout the host program lifetime. This change caches the context offline device setting in the platform which saves the overhead of calling the environment variable querying function.

[pcolberg]

Thanks @sophimao for addressing this performance issue. The change is headed in the right direction, but post feature complete I would suggest specifically addressing the regression from #214 and postponing more extensive refactoring to the next release.

What do you think about breaking this up into two PRs?

1. Open a second PR for the regression fix with three commits:

   a. declare and initialize acl_platform.offline_mode, using int as before,

   b. replace calls of (void) acl_get_offline_device_user_setting(...) with acl_platform.offline_mode; in the commit message explain why the caching is needed,

   c. introduce and declare acl_platform.offline_mode as an enum.

2. Mark this PR as a draft for the next release, to be rebased on the other PR once merged.

In particular, for the regression fix I would suggest leaving the current signature of acl_get_offline_device_user_setting() intact, apart from switching to an enum which has no further implications.

Regarding the offline_device string, however, I am wondering whether the post-release refactoring PR could replace this with a non-string representation of the possible states, e.g., replacing the + prefixing with a combination of enums or something to that effect.

Series of small commits are much easier to review without the context you gained from implementing the change, and they allow individual backporting or reversal as needed.

[pcolberg]

1c. introduce and declare acl_platform.offline_mode as an enum.

Thinking about it some more, you might even want to postpone this change to later. It does not do any harm, but you generally want to avoid introducing a change that will likely require further refactoring anyway, e.g., to replace the offline_device string with a less error-prone representation.

[sophimao]

Thanks Peter for the review! I actually have a separate change just targeting the QoR regression introduced in #214, just that I didn't include the enum change in there. I will modify that a bit more to replace all the acl_get_offline_device_user_setting calls and create another PR, then change this PR to a draft.

[pcolberg]

Thanks @sophimao for resuming this. I suggest to focus on acl_get_offline_device_user_setting() first. That function is hard to follow due to the deep nesting. Would it be feasible to simplify the control flow such that each possible mode is handled in a single block and within each block the function immediately returns the mode and device name?

[pcolberg]

You can introduce a scoped enum to group the constants:

Suggested change

	typedef enum {
	ACL_CONTEXT_OFFLINE_AND_AUTODISCOVERY,
	ACL_CONTEXT_OFFLINE_ONLY,
	ACL_CONTEXT_MSIM,
	ACL_CONTEXT_MPSIM
	} acl_context_offline_mode_t;
	enum class acl_context_offline_mode {
	offline_and_autodiscovery,
	offline_only,
	msim,
	mpsim
	};

Lowercase is easier to discern for the human reader. (Other projects use SnakeCase for similar reasons.)

[pcolberg]

How about combining these into a struct to avoid the null pointer?

struct acl_context_offline_device {
  std::string name;
  acl_context_offline_mode mode;
}

[pcolberg]

Yes, this function is indeed the right place for this code. However, could you replace the earlier setting = "1" (which is confusing since that does not appear to be an actually used device name) in favour of directly setting the device name to ACL_MPSIM_DEVICE_NAME?

What is the content of CL_CONTEXT_MSIM_DEVICE_INTELFPGA? Is that a boolean or an actual device name? I could not find an equivalent ACL_MSIM_DEVICE_NAME. Is the code for the old simulator still needed at all?

[pcolberg]

This does not document the possible modes, but the offline device name passed to acl_get_offline_device_user_setting(). Could you move this comment to the function body and replace the existing, duplicate comment? (There are two possible places for the comment; I prefer the function definition since that avoids having to jump between the comment and the implementation.)