Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings
/xml2Public

Use cpp11#405

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
mgirlich wants to merge27 commits intor-lib:main
base:main
Choose a base branch
Loading
frommgirlich:use-cpp11
Open

Use cpp11#405

mgirlich wants to merge27 commits intor-lib:mainfrommgirlich:use-cpp11

Conversation

@mgirlich
Copy link
Contributor

@mgirlichmgirlich commentedSep 4, 2023•
edited
Loading

const xmlChar* uri = (xmlChar*)Rf_translateCharUTF8(STRING_ELT(x_sxp, i));
SET_STRING_ELT(out, i,Xml2String(xmlBuildURI(uri, base_uri)).asRString());
const xmlChar* uri = (xmlChar*)Rf_translateCharUTF8(x_sxp[i]);
out[i] =Xml2String(xmlBuildURI(uri, base_uri)).asRString();
Copy link
ContributorAuthor

@mgirlichmgirlichSep 4, 2023•
edited
Loading

Choose a reason for hiding this comment

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

I guess the slow down due to cpp11 protection probably doesn't matter so much here (https://cpp11.r-lib.org/articles/FAQ.html#ok-but-i-really-want-to-call-cpp11unwind_protect-manually).

Copy link
Member

Choose a reason for hiding this comment

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

Yea if it isn't critical then I'd probably leave as is

for (int i =0; i < n; ++i) {
const xmlChar* uri = (xmlChar*)Rf_translateCharUTF8(STRING_ELT(x_sxp, i));
SET_STRING_ELT(out, i,Xml2String(xmlBuildURI(uri, base_uri)).asRString());
const xmlChar* uri = (xmlChar*)Rf_translateCharUTF8(x_sxp[i]);
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Probably we can somehow leave away theRf_translateCharUTF8() due to using cpp11.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don'tthink cpp11 attempts to convert to UTF8 in this case

It looks like if we went through an intermediatestd::string it would translate, but what you currently have seems ok
https://github.com/r-lib/cpp11/blob/500f642b4ea132ec8c168fc70a28e81e9510ece3/inst/include/cpp11/r_string.hpp#L29

mgirlich reacted with thumbs up emoji
if (Rf_xlength(base_sxp) >1) {
Rf_error("Base URL must be length 1");
const xmlChar*to_xml_chr(cpp11::strings x,constchar* arg) {
if (x.size() >1) {
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Might make more sense to check this in R.

SEXPread_bin(SEXPcon,size_tbytes=64*1024);
SEXPwrite_bin(SEXPdata,SEXPcon);

inlineSEXPR_GetConnection(SEXPcon) {returncon; }
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I removedR_GetConnection() as it seemed unnecessary.


inlineSEXPR_GetConnection(SEXPcon) {returncon; }

inlinesize_tR_ReadConnection(SEXPcon,void*buf,size_tn) {
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

R_ReadConnection() wasn't used anywhere.

}
cpp11::sexpwrite_bin(cpp11::sexpdata,cpp11::sexpcon);

inlinesize_tR_WriteConnection(SEXPcon,void*buf,size_tn) {
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This usesmemcpy() and I wasn't sure how to do this properly with cpp11.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is an alternative tomemcpy(), but you could allocate a cpp11raws and then dopayload.sexp() to get access to the SEXP you can callRAW() on. I don't think that is too awful, and at least lets cpp11 manage the memory

}
cpp11::sexpwrite_bin(cpp11::sexpdata,cpp11::sexpcon);

inlinesize_tR_WriteConnection(SEXPcon,void*buf,size_tn) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is an alternative tomemcpy(), but you could allocate a cpp11raws and then dopayload.sexp() to get access to the SEXP you can callRAW() on. I don't think that is too awful, and at least lets cpp11 manage the memory

extern"C" SEXPunique_ns(SEXP ns) {
[[cpp11::register]]
cpp11::sexpunique_ns(SEXP ns) {
BEGIN_CPP
Copy link
Member

Choose a reason for hiding this comment

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

Remove?

Copy link
Member

Choose a reason for hiding this comment

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

And in the other places below. Do we need this macro at all anymore? I assume if you go through cpp11 everywhere it will wrap it in try/catch for you

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@DavisVaughan If you could merge#400 then I could also finish xml2_node.cpp and I think then it should be safe to remove the macros.

for (int i =0; i < n; ++i) {
const xmlChar* uri = (xmlChar*)Rf_translateCharUTF8(STRING_ELT(x_sxp, i));
SET_STRING_ELT(out, i,Xml2String(xmlBuildURI(uri, base_uri)).asRString());
const xmlChar* uri = (xmlChar*)Rf_translateCharUTF8(x_sxp[i]);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don'tthink cpp11 attempts to convert to UTF8 in this case

It looks like if we went through an intermediatestd::string it would translate, but what you currently have seems ok
https://github.com/r-lib/cpp11/blob/500f642b4ea132ec8c168fc70a28e81e9510ece3/inst/include/cpp11/r_string.hpp#L29

mgirlich reacted with thumbs up emoji
const xmlChar* uri = (xmlChar*)Rf_translateCharUTF8(STRING_ELT(x_sxp, i));
SET_STRING_ELT(out, i,Xml2String(xmlBuildURI(uri, base_uri)).asRString());
const xmlChar* uri = (xmlChar*)Rf_translateCharUTF8(x_sxp[i]);
out[i] =Xml2String(xmlBuildURI(uri, base_uri)).asRString();
Copy link
Member

Choose a reason for hiding this comment

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

Yea if it isn't critical then I'd probably leave as is

Comment on lines -68 to +73
SET_STRING_ELT(scheme, i,Rf_mkChar(uri->scheme ==NULL ?"" : uri->scheme));
SET_STRING_ELT(server, i,Rf_mkChar(uri->server ==NULL ?"" : uri->server));
INTEGER(port)[i] = uri->port ==0 ? NA_INTEGER : uri->port;
SET_STRING_ELT(user, i,Rf_mkChar(uri->user ==NULL ?"" : uri->user));
SET_STRING_ELT(path, i,Rf_mkChar(uri->path ==NULL ?"" : uri->path));
SET_STRING_ELT(fragment, i,Rf_mkChar(uri->fragment ==NULL ?"" : uri->fragment));
scheme[i] =uri->scheme ==NULL ?"" : uri->scheme;
server[i] =uri->server ==NULL ?"" : uri->server;
port[i] = uri->port ==0 ? NA_INTEGER : uri->port;
user[i] =uri->user ==NULL ?"" : uri->user;
path[i] =uri->path ==NULL ?"" : uri->path;
fragment[i] =uri->fragment ==NULL ?"" : uri->fragment;
Copy link
Member

Choose a reason for hiding this comment

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

This one might get significantly slower, but again it doesn'tseem like it is used all that often? I couldn't really say

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I also think it shouldn't be used that often.

@mgirlich
Copy link
ContributorAuthor

@DavisVaughan I now also migratedxml2_node.cpp to cpp11. But the performance hit from string handling really bites us now. It is nearly as slow as before replacing the S3 dispatch.

@DavisVaughan
Copy link
Member

Is it one or two functions in particular?

@mgirlich
Copy link
ContributorAuthor

I haven't looked into this in more detail but only checkedxml_name() (see the benchmark in#400). The easiest would probably be to check and carefully revert some of the changes and then wrap things withcpp11::unwind_protect().

@jeroenjeroenforce-pushed themain branch 2 times, most recently from8f62602 to9c3fc65CompareDecember 16, 2024 00:06
@jeroenjeroenforce-pushed themain branch 3 times, most recently from463c7e9 toece90bbCompareMarch 1, 2025 19:42
@jeroenjeroenforce-pushed themain branch 2 times, most recently from88024a3 tof9c5fe3CompareAugust 20, 2025 10:52
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@DavisVaughanDavisVaughanDavisVaughan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mgirlich@DavisVaughan
[8]ページ先頭
©2009-2025 Movatter.jp