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

Added selective async APIs to H5M, H5R, and H5T #96

Closed
wants to merge 3 commits into from
Closed

Added selective async APIs to H5M, H5R, and H5T #96

wants to merge 3 commits into from

Conversation

bmribler
Copy link
Contributor

No tests added yet

Platforms tested:
Linux/64 (jelly)

    No tests added yet
Platforms tested:
    Linux/64 (jelly)
Copy link
Contributor

@qkoziol qkoziol left a comment

Choose a reason for hiding this comment

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

Definitely in good shape, just a few things to adjust / update.


/* If a token was created, add the token to the event set */
if (NULL != token)
if (H5ES_insert(es_id, vol_obj->connector, token,
Copy link
Contributor

Choose a reason for hiding this comment

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

I added some additional cleanup when H5ES_insert() fails. Can you look at H5Dcreate_async, and add the H5I_dec_app_ref_always_close() piece?

if (NULL != token)
if (H5ES_insert(es_id, vol_obj->connector, token,
H5ARG_TRACE11(FUNC, "*s*sIui*siiiiii", app_file, app_func, app_line, loc_id, name, key_type_id, val_type_id, lcpl_id, mcpl_id, mapl_id, es_id)) < 0)
HGOTO_ERROR(H5E_MAP, H5E_CANTINSERT, FAIL, "can't insert token into event set")
Copy link
Contributor

Choose a reason for hiding this comment

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

Return value for error macro should be H5I_INVALID_HID


/* If a token was created, add the token to the event set */
if (NULL != token)
if (H5ES_insert(es_id, vol_obj->connector, token,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, for error cleanup (H5I_dec_app_ref_always_close) and error return value.

@@ -461,6 +606,61 @@ H5Mclose(hid_t map_id)
FUNC_LEAVE_API(ret_value)
} /* end H5Mclose() */

/*-------------------------------------------------------------------------
* Function: H5Mclose
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the name needs to change here (in the comment) and in the code

token_ptr = &token;

/* Add key-value pair to the map asynchronously */
if ((ret_value = H5M__put_api_common(map_id, key_mem_type_id, key, val_mem_type_id, value, dxpl_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't set the ret_value from the common routine's return value.


/* If a token was created, add the token to the event set */
if (NULL != token)
if (H5ES_insert(es_id, vol_obj->connector, token,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for H5I_dec_app_ref_always_close() cleanup here, on error.


/* If a token was created, add the token to the event set */
if (NULL != token)
if (H5ES_insert(es_id, vol_obj->connector, token,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, for H5I_dec_app_ref_always_close and H5I_INVALID_HID on error.


/* If a token was created, add the token to the event set */
if (NULL != token)
if (H5ES_insert(es_id, vol_obj->connector, token,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for H5I_dec_app_ref_always_close() on error.

@@ -501,12 +501,15 @@ H5_DLLVAR hid_t H5T_NATIVE_UINT_FAST64_g;
/* Operations defined on all datatypes */
H5_DLL hid_t H5Tcreate(H5T_class_t type, size_t size);
H5_DLL hid_t H5Tcopy(hid_t type_id);
H5_DLL hid_t H5Tcopy_async(const char *app_file, const char *app_func, unsigned app_line, hid_t obj_id, hid_t es_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Arguments and macros.

HDassert(loc_params);

/* Get object token */
/* This caused the need to include H5Rpkg.h, which is not allowed. Right?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'd be pretty reluctant to include H5Rpkg.h in this file. Is there some other way to structure this routine's arguments?

@qkoziol
Copy link
Contributor

qkoziol commented Nov 16, 2020

Definitely in good shape, just a few things to adjust / update.

Also - you should pull the current version of the branch, which will help reveal problems.

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.

2 participants