You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "Donny9 (via GitHub)" <gi...@apache.org> on 2023/09/04 10:24:07 UTC
[GitHub] [nuttx] Donny9 opened a new pull request, #10475: libs/libc: support unlock version for fread/fwrite/fputc/fgetc/...
Donny9 opened a new pull request, #10475:
URL: https://github.com/apache/nuttx/pull/10475
## Summary
libs/libc: support unlock version for fread/fwrite/fputc/fgetc/... to improve performance.
## Impact
add unlock api for fread/fwrite/fpuc/fgetc/fgets/fputs/feof/ferrno/clearerr/fileno/getc/getchar/putc/putchar/
## Testing
local test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #10475: libs/libc: support unlock version for fread/fwrite/fputc/fgetc/...
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10475:
URL: https://github.com/apache/nuttx/pull/10475#discussion_r1316251813
##########
libs/libc/stdio/lib_rdflush_unlocked.c:
##########
@@ -98,12 +94,10 @@ int lib_rdflush(FAR FILE *stream)
if (fseek(stream, -rdoffset, SEEK_CUR) < 0)
Review Comment:
spec doesn't define fseek_unlock
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] Donny9 commented on a diff in pull request #10475: libs/libc: support unlock version for fread/fwrite/fputc/fgetc/...
Posted by "Donny9 (via GitHub)" <gi...@apache.org>.
Donny9 commented on code in PR #10475:
URL: https://github.com/apache/nuttx/pull/10475#discussion_r1316697726
##########
libs/libc/stdio/lib_getc.c:
##########
@@ -32,3 +32,8 @@ int getc(FAR FILE *stream)
{
return fgetc(stream);
}
+
+int getc_unlocked(FAR FILE *stream)
Review Comment:
@pkarashchenko The PR https://github.com/apache/nuttx/pull/1321 move getc/putc these api to c souce, maybe it's because of C++ problem
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] Donny9 commented on a diff in pull request #10475: libs/libc: support unlock version for fread/fwrite/fputc/fgetc/...
Posted by "Donny9 (via GitHub)" <gi...@apache.org>.
Donny9 commented on code in PR #10475:
URL: https://github.com/apache/nuttx/pull/10475#discussion_r1316675197
##########
libs/libc/stdio/lib_getchar.c:
##########
@@ -38,3 +38,13 @@ int getchar(void)
return read(STDIN_FILENO, &c, 1) == 1 ? c : EOF;
#endif
}
+
+int getchar_unlocked(void)
+{
+#ifdef CONFIG_FILE_STREAM
+ return fgetc_unlocked(stdin);
+#else
+ unsigned char c;
+ return read(STDIN_FILENO, &c, 1) == 1 ? c : EOF;
Review Comment:
kernel don't call getchar and putchar, only userspace call them, so keey read? ditto.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] Donny9 commented on a diff in pull request #10475: libs/libc: support unlock version for fread/fwrite/fputc/fgetc/...
Posted by "Donny9 (via GitHub)" <gi...@apache.org>.
Donny9 commented on code in PR #10475:
URL: https://github.com/apache/nuttx/pull/10475#discussion_r1316967087
##########
libs/libc/stdio/lib_rdflush_unlocked.c:
##########
@@ -98,12 +94,10 @@ int lib_rdflush(FAR FILE *stream)
if (fseek(stream, -rdoffset, SEEK_CUR) < 0)
Review Comment:
Done, using lseek to do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #10475: libs/libc: support unlock version for fread/fwrite/fputc/fgetc/...
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10475:
URL: https://github.com/apache/nuttx/pull/10475#discussion_r1315477043
##########
include/stdio.h:
##########
@@ -141,14 +146,20 @@ int feof(FAR FILE *stream);
int ferror(FAR FILE *stream);
int fileno(FAR FILE *stream);
int fgetc(FAR FILE *stream);
+int fgetc_unlocked(FAR FILE *stream);
int fgetpos(FAR FILE *stream, FAR fpos_t *pos);
FAR char *fgets(FAR char *s, int n, FAR FILE *stream);
+FAR char *fgets_unlocked(FAR char *s, int n, FAR FILE *stream);
FAR FILE *fopen(FAR const char *path, FAR const char *type) fopen_like;
int fprintf(FAR FILE *stream, FAR const IPTR char *format, ...)
printf_like(2, 3);
int fputc(int c, FAR FILE *stream);
+int fputc_unlocked(int c, FAR FILE *stream);
int fputs(FAR const IPTR char *s, FAR FILE *stream);
+int fputs_unlocked(FAR const IPTR char *s, FAR FILE *stream);
size_t fread(FAR void *ptr, size_t size, size_t n_items, FAR FILE *stream);
+size_t fread_unlocked(FAR void *ptr, size_t size, size_t n_items,
+ FAR FILE *stream);
Review Comment:
```suggestion
size_t fread_unlocked(FAR void *ptr, size_t size, size_t n_items,
FAR FILE *stream);
```
##########
include/stdio.h:
##########
@@ -160,8 +171,11 @@ long ftell(FAR FILE *stream);
off_t ftello(FAR FILE *stream);
size_t fwrite(FAR const void *ptr, size_t size, size_t n_items,
FAR FILE *stream);
+size_t fwrite_unlocked(FAR const void *ptr, size_t size, size_t n_items,
+ FAR FILE *stream);
Review Comment:
```suggestion
size_t fwrite(FAR const void *ptr, size_t size, size_t n_items,
FAR FILE *stream);
size_t fwrite_unlocked(FAR const void *ptr, size_t size, size_t n_items,
FAR FILE *stream);
```
##########
libs/libc/stdio/lib_getc.c:
##########
@@ -32,3 +32,8 @@ int getc(FAR FILE *stream)
{
return fgetc(stream);
}
+
+int getc_unlocked(FAR FILE *stream)
Review Comment:
Just a note for the future discussion: Since `getc` alywas relies on `fgetc` then maybe we can make it as a defined in stdio.h? Especially POSIX states
```
The getc() function shall be equivalent to fgetc, except that if it is implemented as a macro it may evaluate stream more than once, so the argument should never be an expression with side-effects.
```
The same can be applied for `putc`
##########
libs/libc/stdio/lib_getchar.c:
##########
@@ -38,3 +38,13 @@ int getchar(void)
return read(STDIN_FILENO, &c, 1) == 1 ? c : EOF;
#endif
}
+
+int getchar_unlocked(void)
+{
+#ifdef CONFIG_FILE_STREAM
+ return fgetc_unlocked(stdin);
+#else
+ unsigned char c;
+ return read(STDIN_FILENO, &c, 1) == 1 ? c : EOF;
Review Comment:
Let's change to
```suggestion
return _NX_READ(STDIN_FILENO, &c, 1) == 1 ? c : EOF;
```
here and in `getchar`
##########
libs/libc/stdio/lib_putchar.c:
##########
@@ -38,3 +38,13 @@ int putchar(int c)
return write(STDOUT_FILENO, &tmp, 1) == 1 ? c : EOF;
#endif
}
+
+int putchar_unlocked(int c)
+{
+#ifdef CONFIG_FILE_STREAM
+ return fputc_unlocked(c, stdout);
+#else
+ unsigned char tmp = c;
+ return write(STDOUT_FILENO, &tmp, 1) == 1 ? c : EOF;
Review Comment:
Let's change to
```suggestion
return _NX_WRITE(STDOUT_FILENO, &tmp, 1) == 1 ? c : EOF;
```
here and in `putchar`
##########
libs/libc/libc.h:
##########
@@ -231,18 +233,21 @@ FAR char *lib_dgets(FAR char *buf, size_t buflen, int fd,
FAR char *lib_fgets(FAR char *buf, size_t buflen, FILE *stream,
bool keepnl, bool consume);
+FAR char *lib_fgets_unlocked(FAR char *buf, size_t buflen, FILE *stream,
+ bool keepnl, bool consume);
Review Comment:
```suggestion
FAR char *lib_fgets_unlocked(FAR char *buf, size_t buflen, FILE *stream,
bool keepnl, bool consume);
```
##########
libs/libc/stdio/lib_rdflush_unlocked.c:
##########
@@ -98,12 +94,10 @@ int lib_rdflush(FAR FILE *stream)
if (fseek(stream, -rdoffset, SEEK_CUR) < 0)
Review Comment:
Do we need unlocked version of `fseek` or replace it with `_NX_SEEK` variant?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #10475: libs/libc: support unlock version for fread/fwrite/fputc/fgetc/...
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10475:
URL: https://github.com/apache/nuttx/pull/10475#discussion_r1317172364
##########
libs/libc/stdio/lib_getc.c:
##########
@@ -32,3 +32,8 @@ int getc(FAR FILE *stream)
{
return fgetc(stream);
}
+
+int getc_unlocked(FAR FILE *stream)
Review Comment:
Ok. Strange since spec says explicitly that those can be a macro. I will explore this deeply. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #10475: libs/libc: support unlock version for fread/fwrite/fputc/fgetc/...
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10475:
URL: https://github.com/apache/nuttx/pull/10475#discussion_r1316416042
##########
libs/libc/stdio/lib_rdflush_unlocked.c:
##########
@@ -98,12 +94,10 @@ int lib_rdflush(FAR FILE *stream)
if (fseek(stream, -rdoffset, SEEK_CUR) < 0)
Review Comment:
Yes, I know it, but it is not logical to call API the is doing `flockfile(stream);` from `_unlocked` API variant, isn't it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] Donny9 commented on pull request #10475: libs/libc: support unlock version for fread/fwrite/fputc/fgetc/...
Posted by "Donny9 (via GitHub)" <gi...@apache.org>.
Donny9 commented on PR #10475:
URL: https://github.com/apache/nuttx/pull/10475#issuecomment-1707605403
> During review I just noticed that `fseeko` relies on `lseek` call and not on `_NX_SEEK` and contain code
>
> ```
> #ifdef CONFIG_DEBUG_FEATURES
> /* Verify that we were provided with a stream */
>
> if (!stream)
> {
> set_errno(EBADF);
> return ERROR;
> }
> #endif
> ```
>
> instead of
>
> ```
> #ifdef CONFIG_DEBUG_FEATURES
> /* Verify that we were provided with a stream */
>
> if (!stream)
> {
> _NX_SETERRNO(EBADF);
> return ERROR;
> }
> #endif
> ```
>
> it would be good if we can fix it
These api with parameter stream handle can't be called in kernel, so don't change these api with _NX_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko merged pull request #10475: libs/libc: support unlock version for fread/fwrite/fputc/fgetc/...
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko merged PR #10475:
URL: https://github.com/apache/nuttx/pull/10475
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] Donny9 commented on a diff in pull request #10475: libs/libc: support unlock version for fread/fwrite/fputc/fgetc/...
Posted by "Donny9 (via GitHub)" <gi...@apache.org>.
Donny9 commented on code in PR #10475:
URL: https://github.com/apache/nuttx/pull/10475#discussion_r1316674382
##########
libs/libc/libc.h:
##########
@@ -231,18 +233,21 @@ FAR char *lib_dgets(FAR char *buf, size_t buflen, int fd,
FAR char *lib_fgets(FAR char *buf, size_t buflen, FILE *stream,
bool keepnl, bool consume);
+FAR char *lib_fgets_unlocked(FAR char *buf, size_t buflen, FILE *stream,
+ bool keepnl, bool consume);
Review Comment:
Done!
##########
include/stdio.h:
##########
@@ -141,14 +146,20 @@ int feof(FAR FILE *stream);
int ferror(FAR FILE *stream);
int fileno(FAR FILE *stream);
int fgetc(FAR FILE *stream);
+int fgetc_unlocked(FAR FILE *stream);
int fgetpos(FAR FILE *stream, FAR fpos_t *pos);
FAR char *fgets(FAR char *s, int n, FAR FILE *stream);
+FAR char *fgets_unlocked(FAR char *s, int n, FAR FILE *stream);
FAR FILE *fopen(FAR const char *path, FAR const char *type) fopen_like;
int fprintf(FAR FILE *stream, FAR const IPTR char *format, ...)
printf_like(2, 3);
int fputc(int c, FAR FILE *stream);
+int fputc_unlocked(int c, FAR FILE *stream);
int fputs(FAR const IPTR char *s, FAR FILE *stream);
+int fputs_unlocked(FAR const IPTR char *s, FAR FILE *stream);
size_t fread(FAR void *ptr, size_t size, size_t n_items, FAR FILE *stream);
+size_t fread_unlocked(FAR void *ptr, size_t size, size_t n_items,
+ FAR FILE *stream);
Review Comment:
Done~
##########
include/stdio.h:
##########
@@ -160,8 +171,11 @@ long ftell(FAR FILE *stream);
off_t ftello(FAR FILE *stream);
size_t fwrite(FAR const void *ptr, size_t size, size_t n_items,
FAR FILE *stream);
+size_t fwrite_unlocked(FAR const void *ptr, size_t size, size_t n_items,
+ FAR FILE *stream);
Review Comment:
Done~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org