Menu
▾
▴
perl-win32-gui-hackers
|
From: Glenn L. <pe...@ne...> - 2004-05-14 04:52:10
|
Per some discussion one ActiveState's perl-win32-users list that you might have seen, I was inspired to make GetOpenFileName work a bit better. The problem at hand is that the results buffer is fixed size, so when selecting lots of files, it would overflow the buffer. Surprisingly, Windows actually detects this instance of buffer overflow, and has a special return value for that case. Win32::GUI's GetOpenFileName wrapper doesn't really handle the special return value, and even if it did, it is not the greatest idea to have the user select his files twice, once causing overflow, and the second time with a bigger buffer. It would be much better to have a bigger buffer to start with. Rather than make a new parameter, it seemed reasonable to me to enhance the -multisel => 0|1 to instead be -multisel => 0|bufsize . So if -multisel is true, the appropriate flags are set as they are today, but also the buffersize is set to the minimum of 256 bytes (today's size) and the bufsize specified in -multisel. This results in 100% compatibility (but a slight slowdown to do a safemalloc/safefree sequence) for people using the 0 or 1 values. But for people that start passing in numbers larger than 256, it provides a larger buffer for multiple selection results. Is there any objections to my applying this patch? In addition to the gui.xs file that I changed, I guess I need to update CHANGELOG. Anything else? Can I change the version to 0.0.681 ? Can we start the convention of bumping the version by 0.0.002 each time we change something (keeping it odd for development versions)? And then by at least one more when we do a build, making it even? Or maybe each build bumps the second 0. So the next "official" release would be 0.1.0. And the next development release after that would be 0.1.001, 0.1.002, etc. Official releases would always be N.M.0 ? -- Glenn -- http://nevcal.com/ =========================== The best part about procrastination is that you are never bored, because you have all kinds of things that you should be doing. |
|
From: Johan L. <jo...@Da...> - 2004-05-14 07:56:51
|
At 06:53 2004-05-14, Glenn Linderman wrote: >has a special return value for that case. Win32::GUI's GetOpenFileName >wrapper doesn't really handle the special return value, and even if it >did, it is not the greatest idea to have the user select his files twice, >once causing overflow, and the second time with a bigger buffer. It would >be much better to have a bigger buffer to start with. In the spirit of Perl, can't we optimize for the common case, and make the uncommon case possible? I'd like to not have to say I don't want a tiny, possibly overflowing buffer. I'd like to have the enhanced, larger buffer by default (say a couple of K at least so it never becomes an issue). And if I really need to go beyond even that, maybe it's time for a new parameter. /J -------- ------ ---- --- -- -- -- - - - - - Johan Lindström Sourcerer @ Boss Casinos johanl AT DarSerMan.com Latest bookmark: "Perl for Windows Distribution main links." http://pacific-design.com/?2,810,10 dmoz (1 of 5): /Computers/Programming/Languages/Perl/ 50 |
|
From: Glenn L. <pe...@ne...> - 2004-05-14 15:46:46
|
On approximately 5/14/2004 12:56 AM, came the following characters from the keyboard of Johan Lindstrom: > At 06:53 2004-05-14, Glenn Linderman wrote: > >> has a special return value for that case. Win32::GUI's >> GetOpenFileName wrapper doesn't really handle the special return >> value, and even if it did, it is not the greatest idea to have the >> user select his files twice, once causing overflow, and the second >> time with a bigger buffer. It would be much better to have a bigger >> buffer to start with. > > > In the spirit of Perl, can't we optimize for the common case, and make > the uncommon case possible? > > I'd like to not have to say I don't want a tiny, possibly overflowing > buffer. I'd like to have the enhanced, larger buffer by default (say a > couple of K at least so it never becomes an issue). And if I really need > to go beyond even that, maybe it's time for a new parameter. "a couple of K" would be 8 times the size of the current buffer. The current buffer is allocated on the C runtime stack at 256 bytes, and according to someone, allows "about 18 files" to be selected... so a couple of K would allow "about 72 files" to be selected. That sounds like plenty for quite a few cases, but I can imagine very practical cases where I personally, would like to allow selection of several hundred files. So I guess GetOpenFileName users have presently been living with the 256 byte buffer. I haven't heard major complaints... of course most of them didn't know -multisel => 1 was even an available option, so maybe that is why. OK, having said all that, is the point of your post that it would be nice to avoid the "malloc/free" calls in many cases? (But remember, this is a user interface function that will soon be waiting for a human response measured in seconds, whereas the malloc/free are measured in microseconds.) Or is the point of your post, that the default allocated buffer should not be 256 (as it was) but should grow to 2K with this code change? Or should grow to 2K if -multisel is non-zero (256 being plenty big enough if -multisel is zero)? Or is the point of your post, that you don't think the value of -multisel should be permitted to be different than 0 or 1, that rather there should be a -resultsbuffersize parameter? Or have I missed the point of your post? I'll be glad to tweak my patch to deal with what people believe to be the common case. -- Glenn -- http://nevcal.com/ =========================== The best part about procrastination is that you are never bored, because you have all kinds of things that you should be doing. |
|
From: Johan L. <jo...@Da...> - 2004-05-14 16:12:51
|
Hehe :) My point is that the module should hide the memory management junk from me. If I use the module, I want the user to select files, I don't want to allocate memory. I don't want to have to care about that. So the default behaviour should work for most people, most of the time. No surprises. The performance overhead in this case can obviously be ignored. The default behaviour should be overridable for those moments where the default buffer is too small or too large. But that's a bonus. /J -------- ------ ---- --- -- -- -- - - - - - Johan Lindström Sourcerer @ Boss Casinos johanl AT DarSerMan.com Latest bookmark: "Oracle Connection Manager Parameters (cman.ora..." <http://download-west.oracle.com/docs/cd/B10501_01/network.920/a96581/cman.htm#496765> dmoz (1 of 7): /Computers/Internet/E-mail/Free/ 9 |
|
From: Glenn L. <pe...@ne...> - 2004-05-14 16:36:15
|
On approximately 5/14/2004 9:12 AM, came the following characters from the keyboard of Johan Lindstrom: > Hehe :) > > My point is that the module should hide the memory management junk from me. > > If I use the module, I want the user to select files, I don't want to > allocate memory. I don't want to have to care about that. So the default > behaviour should work for most people, most of the time. No surprises. > The performance overhead in this case can obviously be ignored. > > The default behaviour should be overridable for those moments where the > default buffer is too small or too large. But that's a bonus. Thanks for the clarification. (Isn't it amazing how many ways things can be interpreted?) So then I think your point, translated to implementation requirements, would be to: 1) Use a 256 byte buffer for -multisel => 0 (why waste memory) 2) Use a 4000 byte buffer for -multisel => 1 (allow more files to be selected normally, even if the user hasn't caught on to doing memory management) 3) For simplicity, and smaller numbers, one could just multiply -multisel by 4000 with a minimum of 256 bytes for the zero case, and achieve all these goals. And a 4000 byte granularity on allocation of the buffer is probably not a big deal for people that need big buffers. I've made these adjustments. I'll hold off committing them until next week, in case there are other comments. -- Glenn -- http://nevcal.com/ =========================== The best part about procrastination is that you are never bored, because you have all kinds of things that you should be doing. |
|
From: Laurent R. <ro...@cl...> - 2004-05-14 18:13:34
|
>
> So then I think your point, translated to implementation requirements,
> would be to:
>
> 1) Use a 256 byte buffer for -multisel => 0 (why waste memory)
> 2) Use a 4000 byte buffer for -multisel => 1 (allow more files to be
> selected normally, even if the user hasn't caught on to doing memory
> management)
> 3) For simplicity, and smaller numbers, one could just multiply
> -multisel by 4000 with a minimum of 256 bytes for the zero case, and
> achieve all these goals. And a 4000 byte granularity on allocation of
> the buffer is probably not a big deal for people that need big buffers.
>
> I've made these adjustments. I'll hold off committing them until next
> week, in case there are other comments.
>
I think it's not a problem to upgrade default stack buffer to 4096 by
default.
Stack allocation is generally same speed for 256 and 4096.
And only dynamicly allocate a buffer when desired size isn't enough.
Something like that :
void
GetOpenFileName(...)
PPCODE:
...
char filename[4096];
...
filename[0] = '\0';
ofn.lpstrFile = filename;
ofn.nMaxFile = 4096;
...
} else if(strcmp(option, "-multisel") == 0) {
next_i = i + 1;
SwitchBit(ofn.Flags, OFN_ALLOWMULTISELECT,
SvIV(ST(next_i)));
if ( ofn.nMaxFile < (MAX_PATH * SvIV(ST(next_i))) ) {
ofn.nMaxFile = MAX_PATH * SvIV(ST(next_i));
ofn.lpstrFile = (char *) safemalloc(ofn.nMaxFile);
ofn.lpstrFile[0] = '\0';
}
}
...
if(retval) {
if (ofn.Flags & OFN_ALLOWMULTISELECT) {
...
if (ofn.lpstrFile != filename)
safefree(ofn.lpstrFile);
XSRETURN(i);
...
Laurent.
|
|
From: Jez W. <je...@je...> - 2004-05-14 19:22:56
|
> I think it's not a problem to upgrade default stack buffer to 4096 by > default. > Stack allocation is generally same speed for 256 and 4096. > And only dynamicly allocate a buffer when desired size isn't enough. > Something like that : > void > GetOpenFileName(...) > PPCODE: > ... > char filename[4096]; > Quick question - are you saying that this 4096 is allocated only when the function is called? Or would it be allocated all the time? If it's the later, why not always dynamically allocate the memory? Does it come down to memory use VS. speed? cheers, jez. |
|
From: Laurent R. <ro...@cl...> - 2004-05-14 19:51:41
|
> > > I think it's not a problem to upgrade default stack buffer to 4096 by > > default. > > Stack allocation is generally same speed for 256 and 4096. > > And only dynamicly allocate a buffer when desired size isn't enough. > > Something like that : > > void > > GetOpenFileName(...) > > PPCODE: > > ... > > char filename[4096]; > > > > Quick question - are you saying that this 4096 is allocated only when the > function is called? Or would it be allocated all the time? If it's the > later, why not always dynamically allocate the memory? Does it come down to > memory use VS. speed? Yes, this 4096 is allocated only when the function is called. It's automaticly reserve space on stack. (it's only one assembler instruction). When you use malloc you call to OS for reserve memory space in HEAP. It's a longer than reserve space on stack. In this case (in GetOpenFilename context) speed difference it's not a problem (Need to wait user interact with dialog). Do, it's possible to always use safemalloc. But, for some high frequency called function, it's better to avoid safemalloc for fixed small size buffer. I arbitrary set 4096 so it's probably enough in much case when using multisel (probably more than 100 files). Laurent |
|
From: Glenn L. <pe...@ne...> - 2004-05-14 20:54:43
|
On approximately 5/14/2004 12:43 PM, came the following characters from the keyboard of Laurent ROCHER: > >>>I think it's not a problem to upgrade default stack buffer to 4096 by >>>default. >>>Stack allocation is generally same speed for 256 and 4096. >>>And only dynamicly allocate a buffer when desired size isn't enough. If performance were critical, or ever important, for this routine, I would agree. I agree anyway, but there is one other consideration.... > In this case (in GetOpenFilename context) speed difference it's not a > problem (Need to wait user interact with dialog). Do, it's possible to > always use safemalloc. The problem is that there are several exit points to the function, and each of them needs to do the buffer cleanup. So making a more complex technique, of having a static buffer, and allocating a dynamic one only when the static one isn't big enough, makes the buffer cleanup routine quite a bit bigger, and it has to be duplicated several times, or else the several exit points merged into one. Neither of those seem appealing, since performance isn't particularly important here. -- Glenn -- http://nevcal.com/ =========================== The best part about procrastination is that you are never bored, because you have all kinds of things that you should be doing. |
|
From: Jez W. <je...@je...> - 2004-05-17 09:06:46
|
> Yes, this 4096 is allocated only when the function is called. > > It's automatically reserve space on stack. (it's only one assembler > instruction). > When you use malloc you call to OS for reserve memory space in HEAP. > It's a longer than reserve space on stack. Thanks for the explanation - makes sense. Cheers, jez. |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X