Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r
On 18-01-2021 15:49, Corinna Vinschen via Cygwin-patches wrote: > > Care to send the resulting patch? > Will send with next patch-set. Ben...
Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r
On Jan 18 14:51, Ben wrote: > On 18-01-2021 14:04, Corinna Vinschen via Cygwin-patches wrote: > > What about this instead? It should be better optimizable: > > > Hmmm: > * _remove_r should still set reent->_errno This is redundant: errno == (*__errno ()) == _REENT->_errno == __getreent ()->errno So ptr->_errno is already set. > * _GLOBAL_REENT isn't threadlocal, what about __getreent() Yeah, that makes sense. Just use the _REENT macro instead. Care to send the resulting patch? Thanks, Corinna
Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r
On 18-01-2021 14:04, Corinna Vinschen via Cygwin-patches wrote:
> What about this instead? It should be better optimizable:
>
Hmmm:
* _remove_r should still set reent->_errno
* _GLOBAL_REENT isn't threadlocal, what about __getreent()
So maybe:
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 4742c6653..9c498cd46 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1122,35 +1122,28 @@ unlink (const char *ourname)
}
extern "C" int
-_remove_r (struct _reent *, const char *ourname)
+_remove_r (struct _reent *ptr, const char *ourname)
{
path_conv win32_name (ourname, PC_SYM_NOFOLLOW);
if (win32_name.error)
{
- set_errno (win32_name.error);
+ ptr->_errno = set_errno (win32_name.error);
syscall_printf ("%R = remove(%s)",-1, ourname);
return -1;
}
- return win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
+ int res = win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
+ if (errno!=0)
+ ptr->_errno = errno;
+ syscall_printf ("%R = remove(%s)", res, ourname);
+ return res;
}
extern "C" int
remove (const char *ourname)
{
- path_conv win32_name (ourname, PC_SYM_NOFOLLOW);
-
- if (win32_name.error)
-{
- set_errno (win32_name.error);
- syscall_printf ("-1 = remove (%s)", ourname);
- return -1;
-}
-
- int res = win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
- syscall_printf ("%R = remove(%s)", res, ourname);
- return res;
+ return _remove_r (__getreent (), ourname);
}
extern "C" pid_t
Ben...
Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r
On Jan 18 13:40, Ben wrote:
> On 18-01-2021 11:56, Corinna Vinschen via Cygwin-patches wrote:
> > Hmm, you're adding another function call to the call stack. Doesn't
> > that slow down _remove_r rather than speeding it up? Ok, this function
> > is called from _tmpfile_r/_tmpfile64_r only, so dedup may trump speed
> > here...
> >
> > What's your stance?
> >
> While I could do without:
> In an earlier version I had changed remove and missed remove_r.
>
> So, this commit is more about de-duplication rather than speed.
What about this instead? It should be better optimizable:
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 4742c665339c..2d8acb4c1052 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1133,24 +1133,15 @@ _remove_r (struct _reent *, const char *ourname)
return -1;
}
- return win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
+ int res = win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
+ syscall_printf ("%R = remove(%s)", res, ourname);
+ return res;
}
extern "C" int
remove (const char *ourname)
{
- path_conv win32_name (ourname, PC_SYM_NOFOLLOW);
-
- if (win32_name.error)
-{
- set_errno (win32_name.error);
- syscall_printf ("-1 = remove (%s)", ourname);
- return -1;
-}
-
- int res = win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
- syscall_printf ("%R = remove(%s)", res, ourname);
- return res;
+ return _remove_r (_GLOBAL_REENT, ourname);
}
extern "C" pid_t
Corinna
Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r
On 18-01-2021 11:56, Corinna Vinschen via Cygwin-patches wrote: > Hmm, you're adding another function call to the call stack. Doesn't > that slow down _remove_r rather than speeding it up? Ok, this function > is called from _tmpfile_r/_tmpfile64_r only, so dedup may trump speed > here... > > What's your stance? > While I could do without: In an earlier version I had changed remove and missed remove_r. So, this commit is more about de-duplication rather than speed. Ben...
Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r
On Jan 15 14:45, Ben Wijen wrote:
> The _remove_r code is already in the remove function.
> Therefore, just call the remove function and make
> sure errno is set correctly in the reent struct.
> ---
> winsup/cygwin/syscalls.cc | 15 ++-
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
> index ce4e9c65c..0e89b4f44 100644
> --- a/winsup/cygwin/syscalls.cc
> +++ b/winsup/cygwin/syscalls.cc
> @@ -1133,18 +1133,15 @@ unlink (const char *ourname)
> }
>
> extern "C" int
> -_remove_r (struct _reent *, const char *ourname)
> +_remove_r (struct _reent *ptr, const char *ourname)
> {
> - path_conv win32_name (ourname, PC_SYM_NOFOLLOW);
> + int ret;
>
> - if (win32_name.error)
> -{
> - set_errno (win32_name.error);
> - syscall_printf ("%R = remove(%s)",-1, ourname);
> - return -1;
> -}
> + errno = 0;
> + if ((ret = remove (ourname)) == -1 && errno != 0)
> +ptr->_errno = errno;
>
> - return win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
> + return ret;
> }
Hmm, you're adding another function call to the call stack. Doesn't
that slow down _remove_r rather than speeding it up? Ok, this function
is called from _tmpfile_r/_tmpfile64_r only, so dedup may trump speed
here...
What's your stance?
Thanks,
Corinna
