Optimize threads #162
Optimize threads #162
Conversation
hloc/extract_features.py
Outdated
| def write_predictions(item, fd, as_half=True): | ||
| name, pred = item | ||
| original_size = pred['image_size'] |
There was a problem hiding this comment.
Style: all code in the project should be indented with 4 spaces
hloc/extract_features.py
Outdated
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Style: only two blank lines between function or class definitions.
hloc/extract_features.py
Outdated
| with tqdm(loader) as pbar: | ||
| for data in pbar: | ||
| process.put(data) |
There was a problem hiding this comment.
Here tqdm will report the time to enqueue but not to process. Could we instead have tqdm accurately report the processing time? for example using the lock defined in tqdm.contrib.concurrent.
There was a problem hiding this comment.
Thanks - will check that out!
hloc/match_features.py
Outdated
| if names_to_pair(*pair) not in skip_pairs] | ||
|
|
||
| feature_pairs = FeaturesPairs(pairs, feature_path_q, feature_paths_refs) | ||
| loader = torch.utils.data.DataLoader(feature_pairs, num_workers=num_workers, batch_size=batch_size, shuffle=False, pin_memory=True) |
There was a problem hiding this comment.
- Does the code really support batch_size>1? This only works if all images have the same number of keypoints, which is not gauranteeed.
- Does batch_size>1 give any performance improvement? With SuperGlue the GPU is usually rather saturated.
There was a problem hiding this comment.
After benchmarking I see the batch_size does not matter - so I've removed it. Performance is almost entirely IO limited, threads for reading and writing take performance doing NN matching from 3 pairs/sec to 33 images / sec
hloc/match_features.py
Outdated
|
|
||
| with tqdm(smoothing=.1, total=len(feature_pairs)) as pbar: | ||
| for pairs, data in loader: | ||
| data = map_tensor(data, partial(torch.Tensor.to, device=device, dtype=torch.float16)) |
There was a problem hiding this comment.
- Why casting the inputs to fp16? Does this speed up NN matching?
- Have you tested this with SuperGlue? Shouldn't the model parameters be also casted to fp16?
- Does this degrade the accuracy in any way? My bet is that you need mixed-precision (with
torch.autocast) because ops like attention probably require the range of fp32.
There was a problem hiding this comment.
I will check that out - mostly I have been using it with torch.cuda.amp.autocast turned on (outside the matcher.main), so I will check with and without (maybe it should be a an argument ... e.g. use_autocast or the like?)
|
Very nice, thank you! I don't have time to test in details for now but I left some high-level preliminary comments. |
| pairs = [pair for pair in set(pairs) | ||
| if names_to_pair(*pair) not in skip_pairs] |
There was a problem hiding this comment.
Here we need to also
- check for existence in
skip_pairsfor both directions 0->1 and 1->0 - Remove duplicated equivalent pairs
There was a problem hiding this comment.
I actually added this in PR #159, I will merge it in the next days
|
Cleaner implementation in #242 |
I've optimized feature extraction and feature matching to make better use of the GPU by reading inputs and writing outputs in threads.