Skip sleep(0.01) for non-change triggered Revise.revise()

[frankier]

Revise.revise() is typically used in one of two ways:

• Called just before code is executed (non-change triggered)
• Called when files are modified (change triggered)

Revise.revise() does a sleep(0.01) in either case

Revise.jl/src/packagedef.jl

Line 751 in 13a5eb7

sleep(0.01) # in case the file system isn't quite done writing out the new files

This is clearly a bit of a hack in the first place, but it does make sense that it might help for change triggered Revise. However, if Revise.revise() is being run in a non-change triggered scenario, there is no reason to think 10ms from now is any better than now. Some non-change triggered uses of Revise, such as the REPL hook, mitigate this by guarding Revise.revise() with isempty(revision_queue), however even if there are revisions, is there any reason to wait 10ms extra in this scenario? It's not so long, but latency adds up.

The simplest solution would be to add a flag e.g. Revise.revise(...; ... skip_sleep=false), and change all non-change triggered usages to set it to true.

[timholy]

Can you give me more information about the non-change triggered uses? I pretty much only use it in the change-triggered case myself. Is this about entr?

[frankier]

Here entr is the change-triggered usage. Most Revise users use non-change triggered Revise I think. (Unless I've misunderstood.)

e.g.

Revise.jl/src/packagedef.jl

Lines 1232 to 1245 in 891b739

	# `revise_first` gets called by the REPL prior to executing the next command (by having been pushed
	# onto the `ast_transform` list).
	# This uses invokelatest not for reasons of world age but to ensure that the call is made at runtime.
	# This allows `revise_first` to be compiled without compiling `revise` itself, and greatly
	# reduces the overhead of using Revise.
	function revise_first(ex)
	# Special-case `exit()` (issue #562)
	if isa(ex, Expr)
	exu = unwrap(ex)
	isa(exu, Expr) && exu.head === :call && length(exu.args) == 1 && exu.args[1] === :exit && return ex
	end
	# Check for queued revisions, and if so call `revise` first before executing the expression
	return Expr(:toplevel, :($isempty($revision_queue) || $(Base.invokelatest)($revise)), ex)
	end

This means that in this case Revise.revise() is run before the user's code is executed in the terminal. This means it is triggered by the user pressing enter rather than a file change.

Here is my reasoning in a bit more depth: My understanding is that the short sleep is really a heuristic to deal with the situation that changes to the filesystem are often non-atomic in practice and bursty like so:

| is a fs change
- is nothing

-------|||-----------------------------|||------------------------------|||------

In the case of entr, we might be woken up directly following a file change. This means there we believe it is reasonably likely we are at the beginning of a burst. Like so:

Case: change triggered
| is a fs change
- is nothing
X We are here

-------|||-----------------------------X||------------------------------|||------

We try and fix this by consolidating the multiple events into a single one. This is sometimes called debouncing. There are different ways we could debounce, but given that each burst is probably short, and they have relatively long gaps between, just waiting a short time after the first one seems reasonable. It might be that this is not logically part of the job really the job of Revise.revise() and should be taken care of somewhere else.

In the case of e.g. revise_first and the REPL, (non-change triggered) I propose that we don't really know where we are in the filesystem change event stream, since we've been triggered by something else:

Case: non-change triggered
| is a fs change
- is nothing
X We are here

-------|||-----------------------------|||------------------------------|||------

XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX (Uniformly random between these)

In this case, the short sleep doesn't appear to help.

Note also that the larger context of this issue (debouncing of fs-changes) has to be considered together with #837 . The current approach of sleeping during the not waiting period seems to depends on later events in the filesystem burst being lost.