pytest-dev / py

Python development support library (note: maintenance only)
MIT License
67 stars 106 forks source link

terminalwriter: cache terminal width (updated on SIGWINCH) #146

Closed blueyed closed 7 years ago

nicoddemus commented 7 years ago

Btw do we have an issue describing why we even need this cache anyway? Is get_terminal_width so expensive as to require a cache?

RonnyPfannschmidt commented 7 years ago

@nicoddemus i just realized that with pytest we maje a major mistake to begin with, terminalwriter doesnt have a "safe" fd

nicoddemus commented 7 years ago

Not sure I follow, can you elaborate?

RonnyPfannschmidt commented 7 years ago

@nicoddemus one of the reasons we need to cache/safe the value is, that we take away the primary file descriptor we use to obtain the terminal size

if we invaldate the cache, on window changes we create a indirect trouble

nicoddemus commented 7 years ago

one of the reasons we need to cache/safe the value is, that we take away the primary file descriptor we use to obtain the terminal size if we invaldate the cache, on window changes we create a indirect trouble

I see thanks @RonnyPfannschmidt. How does this affect this PR in py though?

RonnyPfannschmidt commented 7 years ago

@nicoddemus as far as i remember this pr was part of getting pytest to cache the width,

@blueyed please verify if that is the case (i may be mistaken)

nicoddemus commented 7 years ago

as far as i remember this pr was part of getting pytest to cache the width,

Because getting the terminal width is slow?

blueyed commented 7 years ago

@RonnyPfannschmidt Yes, it is coming through pytest.

Plugins like pytest-sugar access this property often, that's why I think a cache makes sense. I've not really benchmarked it, but communicating with the terminal is rather slow in general I assume.

RonnyPfannschmidt commented 7 years ago

@blueyed then we need to take care of what happens when the primary terminal fd is moved away

nicoddemus commented 7 years ago

I did a quick benchmark of get_terminal_width() and here's what I got:

Python 3.5 windows

> python -m timeit -s "from py._io.terminalwriter import get_terminal_width" "get_terminal_width()"
1000 loops, best of 3: 708 usec per loop

Python 3.5 linux

python -m timeit -s "from py._io.terminalwriter import get_terminal_width" "get_terminal_width()"
100000 loops, best of 3: 6.17 usec per loop

I'm not sure we need to bother with a cache at all, what do you guys think?

RonnyPfannschmidt commented 7 years ago

@nicoddemus the worse problem is, that this code always and unconditionally uses the FD 1 - so its inherently broken for py.test

nicoddemus commented 7 years ago

@RonnyPfannschmidt I see, but how would caching help the situation?

The-Compiler commented 7 years ago

Not worth adding a cache for something which happens seldom and takes way under 1ms, IMHO.

RonnyPfannschmidt commented 7 years ago

@nicoddemus caching wouldnt help at all, same goes for py.test storing it

nicoddemus commented 7 years ago

@RonnyPfannschmidt I see thanks.

I guess we can scrap the idea of caching get_terminal_width() at all then?

RonnyPfannschmidt commented 7 years ago

yup

nicoddemus commented 7 years ago

Thanks anyway @blueyed! 😁